Add compute_units DPC++ FPGA tutorial #167

jessicadavies-intel · 2020-09-18T16:17:33Z

Description

This tutorial shows how to make multiple copies of kernels, called compute_units.

Type of Change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Command Line

jessicadavies-intel · 2020-09-18T16:20:47Z

@akertesz @AlbyB @pmpeter1 @tomlenth please review.

akertesz · 2020-09-18T17:59:57Z

Your commit includes several superfluous files -- a.out in the src directory, and a bunch of binary files. Please remove these.

pmpeter1

The #include "dpc_common.hpp" needs a comment showing the location in the basetoolkit for this file.

akertesz · 2020-09-18T20:21:46Z

@pmpeter1 -- The README contains text indicating where to find the dpc_common.hpp file. You want to see this in the *cpp as well?

pmpeter1 · 2020-09-18T20:46:13Z

Yes, the coding guidelines recommend a 1-line comment showing the path to the directory. The way we are delivering shared headers is a bit awkward so having it in both places is recommended.

jessicadavies-intel · 2020-09-18T21:02:18Z

@akertesz thank you for pointing me to the unrelated binary files that got deleted accidentally. I used a hard rebase to clean up the branch, which closed this PR automatically.

The new commit includes the dpc_common.h comment requested by @pmpeter1

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.cpp

tomlenth · 2020-09-18T21:32:59Z

Readme looks fine. thank you

akertesz · 2020-09-18T21:45:18Z

Shouldn't merge this until Paul approves, and we sort out why CI isn't running.

JoeOster

Need to figure out ci failure, here are a part of the error logs
Note: We should be targeting c++17
.
dpcpp /GX -fintelfpga -DFPGA_EMULATOR -std=c++14 compute_units.cpp -o compute_units.fpga_emu.exe

dpcpp: warning: unknown argument ignored in clang-cl '-std=c++14'; did you mean '-Qstd=c++14'? [-Wunknown-argument]

In file included from compute_units.cpp:13:

./pipe_array.hpp(38,19): error: 'INTEL' is not a class, namespace, or enumeration

    cl::sycl::INTEL::pipe<StructId<idxs...>, BaseTy, depth>;

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/README.md

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/pipe_array.hpp

keryell

Nice to have a more complex FPGA example.
Please explain more the code with some comments, since this is a tutorial.
In intel/llvm#2395 I have already reviewed this code with some suggestions about some syntax simplification for your pipe_array.
@mkinsner @jbrodman I would love simpler code when this is about FPGA... :-)

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/CMakeLists.txt

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/build.ninja

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/README.md

keryell · 2020-09-22T21:47:42Z

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/pipe_array.hpp

+
+template <class Id, typename BaseTy, size_t depth, size_t... dims>
+struct PipeArray {
+  PipeArray() = delete;


Objects of type PipeArray should never be created. This line makes it impossible to accidentally construct PipeArrays, by disabling the default constructor for PipeArray. Since we didn't add any other constructors, it won't be possible to construct PipeArray at all.

Thanks. But what I mean is: explain this in a comment. :-) Keep it for your next improvement. This is a tutorial, so anyone interested by FPGA (usually not super C++-savvy) should be able to understand this code.

Is this the definition of a singleton class?

No. Just that this class is only used as a meta-function for meta-programming and its instantiation has no usage.
The problem here is that since nothing is explained, a user trying to create such an object could not, but not knowing why, since there is no static_assert or comment to clarify the reason.
On the other hand, creating such an object has no effect, so at the end this is a lot of work to just add some smoke curtain... Perhaps just removing this line is good? :-)

I agree this is confusing. Having a class called PipeArray but you cannot make an object seems, at best, counterintuitive. I would think I could make as many of these as I want... Maybe we can figure something cleaner out later @jessicadavies-intel

keryell · 2020-09-22T21:48:52Z

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/pipe_array_internal.hpp

+template <size_t dim1, size_t... dims>
+struct VerifierDimLayer {
+  template <size_t idx1, size_t... idxs>
+  struct VerifierIdxLayer {


Ask a random colleague or customer to explain this code. If there are some hesitations, there might be some comments lacking... ;-)

Yes, I will add comments here, in the separate change I'm planning to make for pipe array improvements.

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/README.md

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.cpp

keryell · 2020-09-22T22:18:02Z

You could remove a + in the PR title Add compute_units DPC+++ FPGA tutorial and also talk about "pipe array"?

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.cpp

akertesz

Approving to re-trigger the CI tests

keryell

A few other clarifications...

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.cpp

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.hpp

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/pipe_array.hpp

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.hpp

keryell

Please express yourself in a lot of comments in the code. :-)

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.hpp

pmpeter1 requested changes Sep 18, 2020

View reviewed changes

JoeOster previously approved these changes Sep 18, 2020

View reviewed changes

jessicadavies-intel closed this Sep 18, 2020

jessicadavies-intel force-pushed the master branch from da855e8 to 8af7dfa Compare September 18, 2020 20:48

Add compute_units DPC++ FPGA tutorial

72ad209

jessicadavies-intel reopened this Sep 18, 2020

jessicadavies-intel dismissed JoeOster’s stale review via 72ad209 September 18, 2020 21:02

pmpeter1 previously approved these changes Sep 18, 2020

View reviewed changes

JoeOster requested changes Sep 18, 2020

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.cpp Show resolved Hide resolved

Add blank line before comment

af6c7e7

jessicadavies-intel dismissed pmpeter1’s stale review via af6c7e7 September 18, 2020 21:12

JoeOster approved these changes Sep 18, 2020

View reviewed changes

pmpeter1 previously approved these changes Sep 18, 2020

View reviewed changes

JoeOster requested changes Sep 18, 2020

View reviewed changes

AlbyB reviewed Sep 19, 2020

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/README.md Show resolved Hide resolved

akertesz reviewed Sep 21, 2020

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/pipe_array.hpp Outdated Show resolved Hide resolved

Fix CI failures

23ad06b

jessicadavies-intel dismissed pmpeter1’s stale review via 23ad06b September 21, 2020 16:50

AlbyB previously approved these changes Sep 21, 2020

View reviewed changes

akertesz mentioned this pull request Sep 21, 2020

Add max_interleaving DPC++ FPGA tutorial #171

Merged

2 tasks

akertesz previously approved these changes Sep 22, 2020

View reviewed changes

jessicadavies-intel requested a review from JoeOster September 22, 2020 17:45

keryell suggested changes Sep 22, 2020

View reviewed changes

keryell reviewed Sep 22, 2020

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.cpp Show resolved Hide resolved

jessicadavies-intel changed the title ~~Add compute_units DPC+++ FPGA tutorial~~ Add compute_units DPC++ FPGA tutorial Sep 23, 2020

Address Ronan's comments

ff8621a

jessicadavies-intel dismissed stale reviews from akertesz and AlbyB via ff8621a September 23, 2020 20:32

akertesz approved these changes Sep 23, 2020

View reviewed changes

JoeOster approved these changes Sep 23, 2020

View reviewed changes

akertesz merged commit 41415dd into oneapi-src:master Sep 23, 2020

keryell reviewed Sep 24, 2020

View reviewed changes

tyoungsc reviewed Sep 24, 2020

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.hpp Show resolved Hide resolved

keryell reviewed Sep 24, 2020

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/DesignPatterns/compute_units/src/compute_units.hpp Show resolved Hide resolved

Add compute_units DPC++ FPGA tutorial #167

Add compute_units DPC++ FPGA tutorial #167

Uh oh!

Conversation

jessicadavies-intel commented Sep 18, 2020

Description

Type of Change

How Has This Been Tested?

Uh oh!

jessicadavies-intel commented Sep 18, 2020

Uh oh!

akertesz commented Sep 18, 2020

Uh oh!

pmpeter1 left a comment

Choose a reason for hiding this comment

Uh oh!

akertesz commented Sep 18, 2020

Uh oh!

pmpeter1 commented Sep 18, 2020

Uh oh!

jessicadavies-intel commented Sep 18, 2020

Uh oh!

Uh oh!

tomlenth commented Sep 18, 2020

Uh oh!

akertesz commented Sep 18, 2020

Uh oh!

JoeOster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

jessicadavies-intel Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyoungsc Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

tyoungsc Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

keryell Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

jessicadavies-intel Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell commented Sep 22, 2020

Uh oh!

Uh oh!

akertesz left a comment

Choose a reason for hiding this comment

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keryell Sep 24, 2020 •

edited

Loading