Skip to content

[SYCL][FPGA] Add support for two new function attributes #3441

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 10 commits into from
Mar 31, 2021

Conversation

smanna12
Copy link
Contributor

This patch implements the support for two new FPGA function attributes:

  1. disable_loop_pipelining
  2. initiation_interval

Both attributes alreday exist for loops as statement attributes.

Function attribute "initiation_interval" takes a single unsigned integer argument.

Syntax:
[[intel::initiation_interval(n)]] void foo() {}

The argument can be constexpr. The parameter n is a template parameter.

The LLVM IR representation will be function metadata:
!initiation_interval !0
!0 = !{!i32 n}

Function attribute "disable_loop_pipelining" takes no arguments.

Syntax:
[[intel::disable_loop_pipelining]] void foo() {}

The LLVM IR representation will be function metadata:
!disable_loop_pipelining !0
!0 = !{!i32 1}

Both attributes are only valid in device code and do not get propagated to the caller.
The function attributes apply to a lambda function, or function definition.

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

Soumi Manna added 5 commits March 29, 2021 21:38
This patch implements the support for two new FPGA function attributes:
  1. disable_loop_pipelining
  2. initiation_interval

Both attributes alreday exist for loops as statement attributes.

Function attribute "initiation_interval" takes a single unsigned integer argument.

Syntax:
[[intel::initiation_interval(n)]] void foo() {}

The argument can be constexpr. The parameter n is a template parameter.

The LLVM IR representation will be function metadata:
!initiation_interval !0
!0 = !{!i32 n}

Function attribute "disable_loop_pipelining" takes no arguments.

Syntax:
[[intel::disable_loop_pipelining]] void foo() {}

The LLVM IR representation will be function metadata:
!disable_loop_pipelining !0
!0 = !{!i32 1}

Both attributes are only valid in device code and do not get propagated to the caller.
The function attributes apply to a lambda function, or function definition.

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 requested a review from AaronBallman March 30, 2021 09:11
let Args = [ExprArgument<"IntervalExpr", /*opt*/1>];
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let HasCustomTypeTransform = 1;
let Documentation = [SYCLIntelFPGAInitiationIntervalAttrDocs];
let SupportsNonconformingLambdaSyntax = 1;
Copy link
Contributor Author

@smanna12 smanna12 Mar 30, 2021

Choose a reason for hiding this comment

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

i checked with feature request author, the attribute needs to apply in this position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking with the authors on these! Just to check -- when you ask their opinion, are you letting them know that a "yes" answer means their attribute isn't valid C++ code? (I have a suspicion that not everyone is thinking about the language design of these attributes so much as what immediate problem they think they're solving. Being asked if they want to add something that's not valid C++ may help them think a bit deeper about the problem.)

let Args = [ExprArgument<"IntervalExpr", /*opt*/1>];
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let HasCustomTypeTransform = 1;
let Documentation = [SYCLIntelFPGAInitiationIntervalAttrDocs];
let SupportsNonconformingLambdaSyntax = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking with the authors on these! Just to check -- when you ask their opinion, are you letting them know that a "yes" answer means their attribute isn't valid C++ code? (I have a suspicion that not everyone is thinking about the language design of these attributes so much as what immediate problem they think they're solving. Being asked if they want to add something that's not valid C++ may help them think a bit deeper about the problem.)

Signed-off-by: Soumi Manna <[email protected]>
@AaronBallman
Copy link
Contributor

This generally LGTM but it's still marked as a draft -- are you planning more changes?

@smanna12 smanna12 marked this pull request as ready for review March 30, 2021 13:06
@smanna12
Copy link
Contributor Author

This generally LGTM but it's still marked as a draft -- are you planning more changes?

No, the feature is complete.

AaronBallman
AaronBallman previously approved these changes Mar 30, 2021
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM aside from a commenting nit.

Signed-off-by: Soumi Manna <[email protected]>
AaronBallman
AaronBallman previously approved these changes Mar 30, 2021
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

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

LGTM

@smanna12
Copy link
Contributor Author

Thanks for the reviews.

@bader bader merged commit 4c6ea6e into intel:sycl Mar 31, 2021
smanna12 added a commit to smanna12/llvm that referenced this pull request Apr 8, 2021
…disable_loop_pipelining attribute

The support for disable_loop_pipelining was implemented on intel#3441

This patch
  1. removes FIXME
  2. Updtaes document
  3. adds tests
  4. fixes typos

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.

4 participants