Conversation
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/sample.json
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_types/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
|
Adding @ajaykumarkannan @tyoungsc @paul-white-intel for review since you were in the review of Abhishek's previous PR. The last round of review in Abhishek's PR was stale. Please review this new PR and leave your comment! :) |
mdbtucker
left a comment
There was a problem hiding this comment.
I believe you were notified on an HSD case regarding the need to initialize ac_ints before using any slc or set_slc operations. I think it is important to include that in this tutorial.
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
|
Shuo, I finished my initial review, but overall I'm not convinced this tutorial does a great job of teaching the user how and when to use ac_ints. I know you inherited this tutorial partially done, perhaps that hand-off is part of the problem. It's not that anything in here is wrong per se, I just feel like we could do a more thorough job demonstrating the use of ac_int, and do a better job of explaining all the 'gotchas' (need to initialize, need for explicit casting, ...). On the other hand, I don't want to let the perfect be the enemy of the good. Having a simple ac_int tutorial is better than no ac_int tutorial. Perhaps we should take an iterative approach? I just worry if we submit this, we won't come back to it. @ajaykumarkannan @tyoungsc your opinions? |
|
Thanks for the review, Mike! You might be right, it's better to simplify this code sample in both README and sample design. |
I don't want to simplify it, I want to expand it quite a bit, add a lot more examples and more comments in the code and the README. |
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
|
We will go along with the iterative approach for this code sample. Here is the plan concluded from the discussion with Mike, Ajay, and Tanner:
|
|
I strongly disagree with your idea of combining all operations into one kernel. As per my previous comment:
|
Let's take a look at the reports that result. I don't think we would create a chain of operations, we'd just put a bunch of independent operations into the same kernel. I don't understand why that is difficult to read in the reports, but if that's the case then let's revisit. The code itself with separate kernels is just so verbose. |
|
That’s acceptable if you include an actual link to the correct place in the FPGA optimization guide.
From: shuoniu-intel ***@***.***>
Sent: Monday, January 24, 2022 10:31 AM
To: shuoniu-intel/oneAPI-samples ***@***.***>
Cc: White, Paul ***@***.***>; Mention ***@***.***>
Subject: Re: [shuoniu-intel/oneAPI-samples] Create AC Int Code Sample (PR #5)
@shuoniu-intel commented on this pull request.
________________________________
In DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md<#5 (comment)>:
+To use this type in your code, you must include the following header:
+
+```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.
+
+For convenience, the following are predefined under the `ac_intN` namespace:
+```
+ac_int<N, true> are type defined as intN up to 63.
+ac_int<N, false> are type defined as uintN up to 63.
+```
+
+For example, a 14 bit signed `ac_int` can be defined by using
+```cpp
+ac_intN::int14 a;
@paul-white-intel<https://github.com/paul-white-intel> Since such content is covered in our documentations, I can add something like "Please refer to the oneAPI DPC++ FPGA Optimization Guide for limitations of ac_int data types." at the end of section "Basic Operations and Promotion Rules". What do you think?
—
Reply to this email directly, view it on GitHub<#5 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AUQVN7PBSI2TNLFMVONV7STUXV5CNANCNFSM5LXIKD4Q>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
|
@paul-white-intel @mdbtucker Here are the two facts I agree with:
Therefore, I propose to have five kernels as following, instead of the current seven:
Compare kernel 1 & 2 to demonstrate
Compare kernel 3 & 4 to demonstrate a shifter generates extra logic if it needs to consider signedness.
Separate these bit manipulation operations in kernel 5 because there is no equivalent operation with native types. What do you think of this proposal? Btw, for your reference, here is the report of the current version of this code sample without my refactoring: https://psg-sc-arc.sc.intel.com/nfs/sc/disks/swuser_work_shuoniu/git/oneAPI-samples/DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/build/ac_int_report.prj/reports/report.html#view1 |
|
I like it. When demonstrating the bit selector please demonstrate initializing the ac_int to something before you start setting individual bits.
From: shuoniu-intel ***@***.***>
Sent: Monday, January 24, 2022 3:04 PM
To: shuoniu-intel/oneAPI-samples ***@***.***>
Cc: White, Paul ***@***.***>; Mention ***@***.***>
Subject: Re: [shuoniu-intel/oneAPI-samples] Create AC Int Code Sample (PR #5)
@paul-white-intel<https://github.com/paul-white-intel> @mdbtucker<https://github.com/mdbtucker>
I gave some more thoughts on how many kernels should we have while I'm refactoring this code sample right now.
Here are the two facts I agree with:
1. It is too verbose to have separate kernels for EACH operation. For example, addition, multiplication, and division can be put in one kernel since resource usage of each operation can be clearly told by the Area report.
2. Some separation is still useful and helpful to demonstrate key concepts listed in the README.
Therefore, I propose to have five kernels as following, instead of the current seven:
* Kernel 1: addition, multiplication, and division using native int.
* Kernel 2: addition, multiplication, and division using ac_int.
Compare kernel 1 & 2 to demonstrate ac_int can improve QoR comparing with native types.
* Kernel 3: ac_int shift like the current ShiftLeft kernel.
* Kernel 4: More efficient ac_int like the current EfficientShiftLeft kernel.
Compare kernel 3 & 4 to demonstrate a shifter generates extra logic if it needs to consider signedness.
* Kernel 5: Demonstrate usage of bit select operator [] and bit slice operations slc and set_slc.
Separate these bit manipulation operations in kernel 5 because there is no equivalent operation with native types.
What do you think of this proposal?
—
Reply to this email directly, view it on GitHub<#5 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AUQVN7KMYVBMDFCL5VOELOLUXW5DDANCNFSM5LXIKD4Q>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/README.md
Outdated
Show resolved
Hide resolved
DirectProgramming/DPC++FPGA/Tutorials/Features/ac_int/src/ac_int.cpp
Outdated
Show resolved
Hide resolved
mdbtucker
left a comment
There was a problem hiding this comment.
Please close this PR and create a PR to the main repo. You may get comments there from non-PSG code samples people, please address them as quickly as possible.
Also, please add me to the Swarm review for the new regtest.
NOTE: This PR continues from Abhishek's work here: tiwaria1#1.
Description
Create new code sample to demonstrate how to use
ac_intdata type.Testing