AC Int Tutorial by tiwaria1 · Pull Request #1 · tiwaria1/oneAPI-samples

tiwaria1 · 2021-10-05T16:02:33Z

Internal review of ac_int tutorial. This is a port of the HLS ac_int/ac_int_basic_ops tutorial.

Testing:
[DONE] Linux Local compile
[TODO] Linux Local compile with CMAKE
[TODO] Linux Regtest
[TODO] Windows Local compile with CMAKE
[TODO] Windows Regtest

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/CMakeLists.txt

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/sample.json

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp

ajaykumarkannan · 2021-10-05T20:43:04Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp

+class SetBitSlice;
+
+using ac_int4 = ac_intN::int4;
+using ac_int14 = ac_intN::int14;


These typedefs seem unnecessary, since we already have them in the ac_int header file.

I will let Paul weigh in on this one as well. I added these because they simplify the functions below and because I have seen non oneAPI code samples do it. I don't have a strong preference either way.

Hi @paul-white-intel please review this part.

I agree with Ajay. I would use typedefs if you wanted to wrap a particular parameterization of ac_int, e.g. ac_int<14, false>, but if you're using the convenience type, then it's kind of redundant. What's the point of the convenience type then? :)

To me, the fewer characters looked better. I didn't want to see ac_intN:: everywhere. If it doesn't add any value in your opinion, I will remove them.

then when you typedef, I think you should use the ac_int<x, b> parameterization.

ajaykumarkannan · 2021-10-05T20:49:25Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp

+    accessor x{inp1, h, read_only};
+    accessor y{inp2, h, read_only};
+    accessor res{result, h, write_only};
+    h.single_task<Mult>([=] { res[0] = x[0] * y[0]; });


Should we instead use one function to demonstrate all of these? I’m worried we’re adding a lot of boilerplate to show something relatively simple.

Perhaps one function per class of operation?

Tanner pointed this out as well. I want to know what Paul thinks about this. One function can be used for the add, sub, mult operations - it could even be just one kernel.

Hi @paul-white-intel please comment on this one as well, thank you!

We can make one function for everything using the right C++ fun. I can chat test something out in Godbolt and send it to Abhishek.

I like having them separate from an IPA point of view; it makes the reports trivial to understand.

An alternative would be to put each of the functions in a separate CPP file, like I did for each of the parts here: https://github.com/intel-sandbox/whitepau.simple-oneapi

I’m worried we’re adding a lot of boilerplate to show something relatively simple.

this is the nature of SYCL. lots of boilerplate.

I'd say no to extra files, since it doesn't actually fix the problem, but rather distribute it to different files.

this is the nature of SYCL. lots of boilerplate.

Untrue. Poorly written SYCL has lots of boilerplate. SYCL has more boilerplate than HLS, sure, but that is because it does more than an HLS component. C++ can have lots of boiler plate too, if it is written poorly. We shouldn't avoid using common C++ features because we are worried about our users not understanding C++. If we think there is a gap, we should explain what is happening instead of avoiding a method to improve code reuse.

ajaykumarkannan · 2021-10-05T20:50:31Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp

+    // create the SYCL device queue
+    queue q(selector, dpc_common::exception_handler);
+
+    // Initialize two random ints


Use a fixed srand() value so that we’re not truly random so it makes it easier to reproduce results here.

I would not use rand() at all... I would want to explicitly show a negative value and a positive value.
Also, why not use ac_int types for t1 and t2 instead of native int types? the truncation should happen automatically.

why not use ac_int types for t1 and t2 instead of native int types?
I think it is to emphasize that the int vs ac_int comparison

How about I replace it with :

int t1 = 624 int t2 = -254 { int golden = t1 + t2; ac_int14 a = t1; ac_int14 b = t2; ac_int15 c; TestAdd(q, a, b, c); ... }

can this be written as

ac_int14 a = 624; ac_int14 b = -254; { int golden = a + b; ac_int15 c; TestAdd(q, a, b, c); ... }

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.hpp

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

whitepau · 2021-10-06T13:56:08Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

+     make fpga
+     ```
+
+3. (Optional) As the above hardware compile may take several hours to complete, FPGA precompiled binaries (compatible with Linux* Ubuntu* 18.04) can be downloaded <a href="https://iotdk.intel.com/fpga-precompiled-binaries/latest/ac_int.fpga.tar.gz" download>here</a>.


do we offer precompiled binaries for all the code samples? I don't recall seeing this before.

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

whitepau · 2021-10-06T15:35:06Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

+```cpp
+#include <sycl/ext/intel/ac_types/ac_int.hpp>
+```
+Additionally, you must use the flag `-qactypes` in order to ensure that the headers are correctly included.


cue the 'You didn't have to do that in HLS' complaints :)

You didn't have to for SYCL before either.
I think the change that resulted in this would affect HLS too, no?

No, HLS branches are separate hence HLS is not affected.

Right. I am saying, if HLS wasn't being deprecated, it would have been updated too.

@tyoungsc that... doesn't make it better.

do you at least get a clear actionable error message if you try to use an ac-type without including -qactypes?

You'd get an include error - it will not be able to find the ac_types header.

hmm. does the error literally say, "make sure you compile with -qactypes"? otherwise, I can see this causing problems.

Unfortunately no. But that is clearly highlighted on the first page of the AC types Documentation. Let me see what we can do to generate that error message.

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

whitepau · 2021-10-06T19:07:57Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

+- If the user knows that the shift direction and that the shift value is always positive, it is advisable to use an unsigned datatype for the shift value to obtain better QoR.
+- Shift values of greater than the width of the datatypes are treated as a shift equal to the width of the datatype.
+
+Further, the operation can be done more efficiently by specifying the amount to shift with the smallest possible ac_int.


this is a similar follow-on to your recommendation to use unsigned datatypes for shift operators, so it should be a bullet in the list above.

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

whitepau · 2021-10-06T19:11:15Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

+## License
+
+Code samples are licensed under the MIT license. See
+[License.txt](https://github.com/oneapi-src/oneAPI-samples/blob/master/License.txt) for details.


do we need a license for the ac_data_types_ref.pdf?

Let me follow up on this.

whitepau · 2021-10-06T19:30:14Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp

+    std::cout << "\nBitwise Operations:\n";
+    // Shift operator
+    {
+      t1 = rand() & ((1 << 13) - 1);


does it make sense to put the testbench code into the TestXXX() functions? that would make the main() function tidier. I believe the main point of separating them in the hls version was to separate the device code from testbench code, but that abstraction doesn't really work in oneAPI anyway, since the TestXXX() functions already contain host code.

Good idea, I think that makes sense. I'll update it once we reach consensus on whether or not to use templated functions for the TestXXX() functions.

You could make functions that accept accessors and then all of the code in the "single_task" lamda would be a call to a function (passing it accessors). Then your functions would be just device code.

whitepau · 2021-10-06T22:34:03Z

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

 | Software                          | Intel® oneAPI DPC++ Compiler <br> Intel® FPGA Add-On for oneAPI Base Toolkit
-| What you will learn               | Using the ac_int data-type for basic operations <br> Efficiently using the left shift operation <br> Setting and reading certain bits of an ac_int number
+| What you will learn               | Using the `ac_int` data-type for basic operations <br> Efficiently using the left shift operation <br> Setting and reading certain bits of an `ac_int` number
 | Time to complete                  | 20 minutes


How do we quantify this 'time to complete'? I think all the code samples are just 20 minutes :)

I view it as time to go through the README and observe the reports.

Correct, it is roughly "time to read to README, not including HW compile time". This is a crude estimate, but gives the reader a decent idea.

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md

tiwaria1 added 5 commits October 5, 2021 08:40

first draft for ac_int tutorial

543779a

add guid, clang format, remove template keyword, add vcxproj flag

f747667

clang format style google

0f2484c

add try catch block

f8799b4

remove unused targets from cmake file

96fa399

tyoungsc reviewed Oct 5, 2021

View reviewed changes

ajaykumarkannan reviewed Oct 5, 2021

View reviewed changes

tiwaria1 added 3 commits October 5, 2021 14:35

remove unnecessary file

650c395

addressed some of the review comments

72e06cd

remove third party programs notice

b2a5c96

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

whitepau reviewed Oct 6, 2021

View reviewed changes

address more review comments

4fc53b4

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

whitepau reviewed Oct 6, 2021

View reviewed changes

DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md Outdated Show resolved Hide resolved

tiwaria1 added 2 commits October 7, 2021 06:57

address some indentation and formatting comments

4c81eaa

do not pass qactypes to report link stage

e3fc988

shuoniu-intel mentioned this pull request Jan 11, 2022

Create AC Int Code Sample shuoniu-intel/oneAPI-samples#5

Closed

4 tasks

Conversation

tiwaria1 commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whitepau Oct 6, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whitepau Oct 6, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whitepau Oct 6, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiwaria1 commented Oct 5, 2021 •

edited

Loading

whitepau Oct 6, 2021 •

edited

Loading

whitepau Oct 6, 2021 •

edited

Loading

whitepau Oct 6, 2021 •

edited

Loading