Skip to content

Create AC Int Code Sample#5

Closed
shuoniu-intel wants to merge 9 commits intomasterfrom
ac-int-tutorial
Closed

Create AC Int Code Sample#5
shuoniu-intel wants to merge 9 commits intomasterfrom
ac-int-tutorial

Conversation

@shuoniu-intel
Copy link
Owner

@shuoniu-intel shuoniu-intel commented Jan 11, 2022

NOTE: This PR continues from Abhishek's work here: tiwaria1#1.

Description

Create new code sample to demonstrate how to use ac_int data type.

Testing

  • Linux manual test (command line compile + HW run)
  • Linux regtest (command line compile + report check + HW run)
  • Windows: manual test (command line compile) (Currently blocked by HSDES: 14015611531)
  • Windows: regtest (command line compile + report check + VS compile) (Currently blocked by HSDES: 14015611531)

@shuoniu-intel shuoniu-intel marked this pull request as ready for review January 11, 2022 20:34
@shuoniu-intel
Copy link
Owner Author

shuoniu-intel commented Jan 11, 2022

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! :)

Copy link
Collaborator

@mdbtucker mdbtucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mdbtucker
Copy link
Collaborator

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?

@shuoniu-intel
Copy link
Owner Author

Thanks for the review, Mike! You might be right, it's better to simplify this code sample in both README and sample design.
The iterative approach works for me, we just need to determine what content do we want to cover in this initial version.

@mdbtucker
Copy link
Collaborator

Thanks for the review, Mike! You might be right, it's better to simplify this code sample in both README and sample design. The iterative approach works for me, we just need to determine what content do we want to cover in this initial version.

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.

@shuoniu-intel
Copy link
Owner Author

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:

  • Improve this code sample for 2022.2:

    • Simplify the code. Don’t put operations in separate kernels.
    • Delete ac_intN from the README and the design code.
    • Address existing comments in the PR.
  • Patch in the next release:

    • Demonstrate ‘gotchas’. E.g., need to initialize for bit select, leverage DSP, etc.

@whitepau
Copy link
Collaborator

I strongly disagree with your idea of combining all operations into one kernel. As per my previous comment:

Personally I like separate kernels because they make the reports easier to parse because you can see the shifters independently of the adders, and slices. (I presume... I did not review the reports. I hope they are clear :))

@mdbtucker
Copy link
Collaborator

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.

@whitepau
Copy link
Collaborator

whitepau commented Jan 24, 2022 via email

@shuoniu-intel
Copy link
Owner Author

shuoniu-intel commented Jan 24, 2022

@paul-white-intel @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?


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

@whitepau
Copy link
Collaborator

whitepau commented Jan 24, 2022 via email

Copy link
Collaborator

@mdbtucker mdbtucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants