Skip to content

[SYCL][FPGA] Add support for new attribute for loop pipelining #6254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jun 11, 2022

Conversation

smanna12
Copy link
Contributor

@smanna12 smanna12 commented Jun 6, 2022

This patch adds support for new FPGA attribute called [[intel::fpga_pipeline(N)]] that takes a Boolean parameter.
N can be a template parameter or constexpr. If the user didn't provide a parameter, the default value of N would
be 1, which implies enable pipelining.

This attribute can be applied to loops.

Example syntax:

// will disable loop pipelining
[[intel::fpga_pipeline(0)]]
for (int i = 0; i<N; ++i) {...}

LLVM IR:
When applied on a loop, the LLVM IR should be attached to llvm.loop metadata:

!{!"llvm.loop.intel.pipelining.enable", i32 N}

The value of the !"llvm.loop.intel.pipelining.enable" metadata is an i32 value that corresponds to the boolean parameter N.
A value of 0 is used for not pipelining the loop.
A value of 1 is used for pipelining the loop.

If the parameter is not given, value of N will be the default value 1, which implies we will pipeline the loop, and the IR will look like

!{!"llvm.loop.intel.pipelining.enable", i32 1}

Error Message:

We should emit an error when [[intel::initiation_interval(k)]] and [[intel::fpga_pipeline(0)]] are used together:

"error: initiation_interval and fpga_pipeline(0) attributes are not compatible"

We should emit an error when [[intel::max_concurrency(k)]] and [[intel::fpga_pipeline(0)]] are used together:

"error: max_concurrency and fpga_pipeline(0) attributes are not compatible"

Note: this basically performs very similar functionalities as [[intel::disable_loop_pipelining]], the plan is to deprecate disable_loop_pipelining in future releases and use [[intel::fpga_pipeline(n)]] instead.

Signed-off-by: Soumi Manna [email protected]

smanna12 added 4 commits June 6, 2022 06:46
This patch adds support for new FPGA attribute called [[intel::fpga_pipeline(N)]] that takes a Boolean parameter.
N can be a template parameter or constexpr. If the user didn't provide a parameter, the default value of N would
be 1, which implies enable pipelining.

This attribute can be applied to loops.

Example syntax:

// will disable loop pipelining
[[intel::fpga_pipeline(0)]]
for (int i = 0; i<N; ++i) {...}

LLVM IR:
When applied on a loop, the LLVM IR should be attached to llvm.loop metadata:

!{!"llvm.loop.intel.pipelining.enable", i32 N}

The value of the !"llvm.loop.intel.pipelining.enable" metadata is an i32 value that corresponds to the boolean parameter N.
A value of 0 is used for not pipelining the loop.
A value of 1 is used for pipelining the loop.

If the parameter is not given, value of N will be the default value 1,  which implies we will pipeline the loop, and the IR will look like

!{!"llvm.loop.intel.pipelining.enable", i32 1}

Error Message:

We should emit an error when [[intel::initiation_interval(k)]] and [[intel::fpga_pipeline(0)]] are used together:

"error: initiation_interval and fpga_pipeline(0) attributes are not compatible"

We should emit an error when [[intel::max_concurrency(k)]] and [[intel::fpga_pipeline(0)]] are used together:

"error: max_concurrency and fpga_pipeline(0) attributes are not compatible"

Note: this basically performs very similar functionalities as [[intel::disable_loop_pipelining]], the plan is to deprecate disable_loop_pipelining in future releases and use [[intel::fpga_pipeline(n)]] instead.

Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
@smanna12 smanna12 marked this pull request as ready for review June 6, 2022 15:10
@smanna12 smanna12 requested a review from a team as a code owner June 6, 2022 15:10
@smanna12 smanna12 requested review from tiwaria1 and AaronBallman June 6, 2022 15:10
@tiwaria1
Copy link
Contributor

tiwaria1 commented Jun 6, 2022

Robert, Ho from PSG (or his delegate) will take over this review :) I have notified him. Removing myself from the review.

@tiwaria1 tiwaria1 removed their request for review June 6, 2022 15:24
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

Just some quick comments; will review again later.

smanna12 added 2 commits June 7, 2022 16:36
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
@smanna12
Copy link
Contributor Author

smanna12 commented Jun 7, 2022

Just some quick comments; will review again later.

Thanks @premanandrao for taking a look at the patch.

@smanna12 smanna12 requested a review from premanandrao June 7, 2022 23:53
@smanna12
Copy link
Contributor Author

smanna12 commented Jun 9, 2022

ping @premanandrao, @elizabethandrews.

smanna12 added 2 commits June 9, 2022 10:46
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
@smanna12 smanna12 requested a review from elizabethandrews June 9, 2022 18:20
premanandrao
premanandrao previously approved these changes Jun 9, 2022
Signed-off-by: Soumi Manna <[email protected]>
Signed-off-by: Soumi Manna <[email protected]>
@smanna12 smanna12 requested a review from premanandrao June 10, 2022 22:03
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

It looks like the new changes since I approved this are the removal of some unneeded testing.
I am fine with these changes.

@pvchupin pvchupin merged commit 92aadf3 into intel:sycl Jun 11, 2022
smanna12 added a commit to smanna12/llvm that referenced this pull request Jul 6, 2022
…ining attribute

intel#6254 added the fpga_pipeline attribute and
deprecated the intel::disable_loop_pipelining attribute.
While this was in the pipeline, the internal decision was made to switch to properties
rather than attributes for this type of thing.

At this point, internal request is that (at a minimum) the deprecation message for
intel::disable_loop_pipelining be removed, as we no longer plan to deprecate it.

Signed-off-by: Soumi Manna <[email protected]>
steffenlarsen pushed a commit that referenced this pull request Jul 7, 2022
…ining attribute (#6404)

#6254 added the fpga_pipeline attribute and
deprecated the intel::disable_loop_pipelining attribute.
While this was in the pipeline, the internal decision was made to switch to properties
rather than attributes for this type of thing.

At this point, internal request is that (at a minimum) the deprecation message for
intel::disable_loop_pipelining be removed, as we no longer plan to deprecate it.
Optionally the intel::fpga_pipeline attribute can be removed from the frontend as well.

Signed-off-by: Soumi Manna <[email protected]>
@smanna12 smanna12 deleted the Add_new-loop-attr branch August 2, 2022 22:09
smanna12 added a commit to smanna12/llvm that referenced this pull request Aug 2, 2022
Support for new FPGA attribute called [[intel::fpga_pipeline(N)]]
on intel#6254 is no longer needed.

This patch removes the support from frontend.

Signed-off-by: Soumi Manna <[email protected]>
bader pushed a commit that referenced this pull request Aug 4, 2022
Support for new FPGA attribute called [[intel::fpga_pipeline(N)]]
on #6254 is no longer needed.

This patch removes the support from frontend.

Signed-off-by: Soumi Manna <[email protected]>
raaiq1 pushed a commit to raaiq1/llvm that referenced this pull request Aug 4, 2022
…#6519)

Support for new FPGA attribute called [[intel::fpga_pipeline(N)]]
on intel#6254 is no longer needed.

This patch removes the support from frontend.

Signed-off-by: Soumi Manna <[email protected]>
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.

5 participants