Skip to content

[SYCL][FPGA] Add support for three new kernel attributes - #3951

Closed
smanna12 wants to merge 1 commit into
intel:syclfrom
smanna12:AddNewAttr
Closed

[SYCL][FPGA] Add support for three new kernel attributes#3951
smanna12 wants to merge 1 commit into
intel:syclfrom
smanna12:AddNewAttr

Conversation

@smanna12

@smanna12 smanna12 commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

This patch adds support for three new kernel attributes:

  1. [[intel::ip_interface_csr]]
  2. [[intel::ip_interface_streaming]]
  3. [[intel::ip_stall_free_return]].

Frontend Specifications:

if the attributes are applied to a function called from a device kernel, the attributes are ignored and they do not get propagated to the kernel. The attributes can only be applied on the device, and not on the host.

The purpose of these attributes is as follows:

ip_interface_csr: the invocation interface of the IP component uses CSR (Control-Status Registers).
ip_interface_streaming: the invocation interface of the IP component supports streaming via a handshaking protocol.
ip_stall_free_return: applies to IP components with a streaming invocation interface. This is an optimization hint that the
return interface of the IP component will never be stalled from downstream.

LLVM IR is function metadata as follows:

If a kernel has ip_interface_csr, function metadata should be on the kernel function:
!ip_interface !0
!0 = !{!”csr”}

If a kernel has ip_interface_streaming, function metadata should be on the kernel function:
!ip_interface !0
!0 = !{!”streaming”}

If a kernel has ip_stall_free_return, function metadata should be on the kernel function:
!ip_interface !0
!0 = !{!”stall_free_return”}

If a kernel has both ip_interface_streaming and ip_stall_free_return, the function metadata should be on the kernel function:
!ip_interface !0
!0 = !{!”streaming”, !”stall_free_return”}

Error Messages:

An error should be output if the kernel attributes ip_interface_csr and ip_interface_streaming are both applied to the same kernel. The error message in this case should be:

          Error: at most one of ip_interface_csr and ip_interface_streaming can be applied to an IP component.

An error should be output if the kernel attributes ip_stall_free_return and ip_interface_csr are both applied to the same kernel,e.g.:

          Error: ip_stall_free_return cannot be applied to an IP component with a CSR interface. Please remove ip_stall_free_return or add ip_interface_streaming to your IP component.

An error should be output if the kernel attribute ip_stall_free_return is applied to a kernel, unless ip_interface_streaming is also applied, e.g.:

          Error: ip_stall_free_return requires ip_interface_streaming. Consider adding ip_interface_streaming to your IP component, or removing ip_stall_free_return.

When a kernel has ip_stall_free_return only, The following part of the original spec can be ignored (No IR in this case) because ip_stall_free_return requires ip_interface_streaming and error will be generated instead.

If a kernel has ip_stall_free_return, function metadata should appear in the IR on the kernel function:
!ip_interface !0
!0 = !{!”stall_free_return”}

Signed-off-by: Soumi Manna soumi.manna@intel.com

@smanna12
smanna12 requested a review from AaronBallman June 17, 2021 17:23
@smanna12
smanna12 marked this pull request as ready for review June 17, 2021 17:59

@AaronBallman AaronBallman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have enough design questions about the attributes that I've not really looked at the implementation heavily yet. I think we need a much clearer understanding of the design here before we proceed.

Comment thread clang/include/clang/Basic/Attr.td Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to stop adding attributes that use non-conforming lambda syntax -- this is not valid C++ code. Is this strictly required? I'm guessing "yes" because it applies to kernels, but this pattern of "well, we already added it one place so now we need to do it everywhere" is really a concern.

Comment thread clang/include/clang/Basic/AttrDocs.td Outdated
Comment on lines 3248 to 3255

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've read all of these words a few times now and I have no idea when to use this attribute, what it actually does, or why this needs to be an attribute in the first place.

It sounds a bit like this is telling the compiler about some information that it should already be able to figure out for itself. Is this attribute necessary?

Why can this attribute not be applied multiple times? What harm is that preventing?

Comment thread clang/include/clang/Basic/AttrDocs.td Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar concerns here -- I have no idea what this attribute actually does from this description. The last one sounded like information the backend can infer rather than requiring the user to mark a function, but with this one I'm not certain I can even guess why it's needed.

Comment thread clang/include/clang/Basic/AttrDocs.td Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is a little bit better in that I at least know it's an optimization hint. How would a user know when to write this attribute, though? I don't know what it means to "never be stalled from downstream", but what's the effect if the user writes this in a situation where the function actually is stalled from downstream?

Comment thread clang/include/clang/Basic/AttrDocs.td Outdated
Comment on lines 3303 to 3306

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, none of these examples really help to understand what the attribute is doing.

Comment on lines 11463 to 11464

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need a new error message for mutually exclusive attributes?

Comment on lines 11465 to 11468

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here.

Comment on lines 11469 to 11471

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we making this the user's problem instead of applying the ip_interface_streaming attribute for the user when they use ip_stall_free_return? (Also, the documentation is not clear that these are required to be in a pair.)

@smanna12

smanna12 commented Jun 18, 2021

Copy link
Copy Markdown
Contributor Author

have enough design questions about the attributes that I've not really looked at the implementation heavily yet. I think we need a much clearer understanding of the design here before we proceed.

Thanks @AaronBallman for taking a look at this PR. I have asked submitter about the design questions you raised here.

@smanna12
smanna12 marked this pull request as draft June 18, 2021 13:20
@smanna12

smanna12 commented Jun 18, 2021

Copy link
Copy Markdown
Contributor Author

Moving this to draft since needs some clarification on spec design.

@keryell

keryell commented Jun 19, 2021

Copy link
Copy Markdown
Contributor

It would be interesting to have for any new attribute proposal a non-attribute C++ counter proposal... :-)
While in HLS C the attributes and #pragma were the norm because of the language limitations, in C++ there might be some C++ friendly work-around...
Specially in SYCL where the philosophy is to have only C++ constructs and no language extension. At the end the attributes can be considered as language extensions...

Signed-off-by: Soumi Manna <soumi.manna@intel.com>
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.

3 participants