[SYCL] Add Clang support for FPGA loop fusion function attributes #2877

elizabethandrews · 2020-12-08T19:15:16Z

This patch adds support for FPGA function attributes loop_fuse and
loop_fuse_independent. [[intel::loop_fuse(N)]] is a strong request,
to the extent possible, to fuse loops within the function, that are
contained in at most N-1 other loops within the function. If the
optional parameter N is omitted, it is a strong request, to the extent
possible, to fuse loops within the function that are not contained in
any other loop within the function. [[intel::loop_fuse_independent(N)]]
is used to guarantee that fusion safety analysis can ignore
negative-distance dependences between these loops.

FrontEnd Specifications:

The attributes take one optional parameter, a constant integral
expression between 0 and 1024*1024. The paramter may be a template
parameter.

The same function definition can have atmost one of these two attributes.

The attributes can be applied explictly to kernel. However, attributes
should not be propagated to callers i.e it should not be propagated from
device functions to kernel.

LLVM IR is function metadata as follows:

define i32 @foo() !loop_fuse !0
!0 = !{i32 N, i32 D}

where N is the value specified by the optional attribute argument. If
the optional argument is omitted, N is set to 1. D is equal to 0 for
[[intel::loop_fuse]] and 1 for [[intel::loop_fuse_independent]].

Signed-off-by: Elizabeth Andrews [email protected]

@foo

This patch adds support for FPGA function attributes loop_fuse and loop_fuse_independent. [[intel::loop_fuse(N)]] is a strong request, to the extent possible, to fuse loops within the function, that are contained in at most N-1 other loops within the function. If the optional parameter N is omitted, it is a strong request, to the extent possible, to fuse loops within the function that are not contained in any other loop within the function. [[intel::loop_fuse_independent(N)]] is used to guarantee that fusion safety analysis can ignore negative-distance dependences between these loops. FrontEnd Specifications: The attributes take one optional parameter, a constant integral expression between 0 and 1024*1024. The paramter may be a template parameter. The same function definition can have atmost one of these two attributes. The attributes can be applied explictly to kernel. However, attributes should not be propagated to callers i.e it should not be propagated from device functions to kernel. LLVM IR is function metadata as follows: define i32 @foo() !loop_fuse !0 !0 = !{i32 N, i32 D} where N is the value specified by the optional attribute argument. If the optional argument is omitted, N is set to 1. D is equal to 0 for [[intel::loop_fuse]] and 1 for [[intel::loop_fuse_independent]]. Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-12-08T20:28:00Z

I was just told this attribute should be disabled for host. I will be pushing a new patch with host support removed. I apologize for the confusion.

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/AttrDocs.td

clang/test/SemaSYCL/loop_fusion.cpp

clang/lib/CodeGen/CodeGenFunction.cpp

clang/include/clang/Basic/Attr.td

clang/lib/CodeGen/CodeGenFunction.cpp

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

clang/lib/Sema/SemaDeclAttr.cpp

AaronBallman · 2020-12-09T17:43:41Z

clang/lib/Sema/SemaDeclAttr.cpp

+  // Handle optional attribute argument.
+  if (Attr.isArgExpr(0))
+    // Attribute argument specified.
+    S.AddOneConstantValueAttr<AttrType>(D, Attr, Attr.getArgAsExpr(0));


Thank you for pointing out where the implementation for that is! FWIW, I would have concerns about upstreaming that function. Nothing for you to solve as part of this review though.

AaronBallman · 2020-12-09T17:49:50Z

clang/lib/Sema/SemaDeclAttr.cpp

+                                                                 Attr))
+    return nullptr;
+
+  if (D->hasAttr<SYCLIntelLoopFuseAttr>())


The usual pattern is: the mergeFooAttr() function checks all of the merging constraints and diagnoses problematic situations like mutually exclusive attributes, duplicate attributes with different arguments, etc, and the handleFooAttr() function checks all of the basic constraints and diagnoses problematic situations like incorrect number of attribute arguments, wrong argument type, etc. and if everything looks okay, calls mergeFooAttr() and only adds the resulting attribute from the merge function if it's nonnull.

The availability attribute is an example of one that has somewhat complicated checking in both handle and merge. What you have in this patch is somewhat close, but the merge function is not diagnosing the duplicate attribute before dropping it, and the handle function is doing a bit of manual work rather than calling the merge function.

clang/lib/Sema/SemaDeclAttr.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

keryell · 2020-12-10T01:35:49Z

I am curious about this D parameter between 0 and 1024*1024...
Why? What is this?

1. Changed attribute definitions to define only 1 semantic attribute 2. Added a couple of tests 3. Minor refactoring/code formatting Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-12-17T15:34:22Z

I am curious about this D parameter between 0 and 1024*1024...
Why? What is this?

N indicates nesting. So it needs to be 0 or more. The upper limit is just an arbitrary high value.

clang/lib/Sema/SemaDeclAttr.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/AttrDocs.td

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/lib/CodeGen/CodeGenFunction.cpp

clang/test/CodeGenSYCL/loop_fusion_host.cpp

during host compilation. Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman

LGTM aside from some minor nits with comments.

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaSYCL.cpp

clang/test/CodeGenSYCL/loop_fusion_host.cpp

elizabethandrews · 2020-12-18T14:35:55Z

Are the buildbots down? @romanovvlad

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-12-18T14:51:01Z

Thanks for the reviews everyone! The final commit just adds missing fullstops.

premanandrao

LGTM (thanks to a thorough review by @AaronBallman), I just had one question below.

clang/test/CodeGenSYCL/loop_fuse_ind_device.cpp

clang/include/clang/Basic/Attr.td

romanovvlad · 2020-12-21T11:14:51Z

Are the buildbots down? @romanovvlad

They should be restored now.

elizabethandrews · 2020-12-21T17:33:35Z

Hmm... That's strange... The test passed locally for me even after I used -triple spir64-unknown-windows-sycldevice

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2020-12-21T17:53:09Z

I think the test fail might have been a name mangling issue. I don't have a Windows workspace at the moment to confirm and I was unable to reproduce the issue on Linux even after using windows triple. Hopefully the latest commit fixes the test failure. If It doesn't I guess I'll make a Windows workspace to try and reproduce the fail.

Semantically we can translate `OpIAddCarry` and `OpISubBorrow` back into `@llvm.uadd.with.overflow` and `@llvm.usub.with.overflow` respectively. It require small transformation as there is a difference between return types in SPIR-V and LLVM IR - e.g., SPIR-V instructions return {i32, i32} struct, but LLVM intrinsics return {i32, i1}. Original commit: KhronosGroup/SPIRV-LLVM-Translator@44bd69ee70b79ed

elizabethandrews requested review from Fznamznon and premanandrao as code owners December 8, 2020 19:15

elizabethandrews mentioned this pull request Dec 8, 2020

[SYCL] Add Clang support for FPGA loop fusion function attributes #2876

Closed

bader requested a review from AaronBallman December 8, 2020 19:39

AaronBallman reviewed Dec 8, 2020

View reviewed changes

AaronBallman reviewed Dec 9, 2020

View reviewed changes

Disable host support.

c94d591

Signed-off-by: Elizabeth Andrews <[email protected]>

Applied review comments -

c6cbe4d

1. Changed attribute definitions to define only 1 semantic attribute 2. Added a couple of tests 3. Minor refactoring/code formatting Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews commented Dec 17, 2020

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

elizabethandrews added 3 commits December 17, 2020 08:07

Revert change which is not required anymore

1b9ce65

Signed-off-by: Elizabeth Andrews <[email protected]>

Merge remote-tracking branch 'intel_llvm/sycl' into loopFusion2

d761846

Fix incorrect conflict resolution

c7ba4b3

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman reviewed Dec 17, 2020

View reviewed changes

Implement review comments.

8c3c5b4

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman reviewed Dec 17, 2020

View reviewed changes

clang/lib/CodeGen/CodeGenFunction.cpp Show resolved Hide resolved

clang/test/CodeGenSYCL/loop_fusion_host.cpp Show resolved Hide resolved

Applied review comment to enable semantic processing of attribute

26b2e52

during host compilation. Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman previously approved these changes Dec 18, 2020

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/loop_fusion_host.cpp Show resolved Hide resolved

Fznamznon previously approved these changes Dec 18, 2020

View reviewed changes

clang/test/CodeGenSYCL/loop_fusion_host.cpp Show resolved Hide resolved

Apply review comments - add missing fullstops in comments

26bbc04

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed stale reviews from Fznamznon and AaronBallman via 26bbc04 December 18, 2020 14:47

elizabethandrews requested a review from Fznamznon December 18, 2020 14:49

Fznamznon previously approved these changes Dec 18, 2020

View reviewed changes

premanandrao previously approved these changes Dec 18, 2020

View reviewed changes

clang/test/CodeGenSYCL/loop_fuse_ind_device.cpp Show resolved Hide resolved

This was referenced Dec 18, 2020

[SYCL] Enable let PragmaAttributeSupport for SYCL attributes #2910

Closed

[SYCL] Investigate arbitrary attribute bounds for FPGA attributes #2911

Closed

[SYCL] Should loop fusion attributes be enabled for C code? #2912

Closed

romanovvlad requested changes Dec 21, 2020

View reviewed changes

clang/include/clang/Basic/Attr.td Show resolved Hide resolved

romanovvlad previously approved these changes Dec 21, 2020

View reviewed changes

Fix Windows test fail

702bed5

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed stale reviews from romanovvlad, premanandrao, and Fznamznon via 702bed5 December 21, 2020 17:50

elizabethandrews requested a review from Fznamznon December 21, 2020 22:23

Fznamznon approved these changes Dec 22, 2020

View reviewed changes

romanovvlad merged commit 23926b0 into intel:sycl Dec 22, 2020

[SYCL] Add Clang support for FPGA loop fusion function attributes #2877

[SYCL] Add Clang support for FPGA loop fusion function attributes #2877

Uh oh!

Conversation

elizabethandrews commented Dec 8, 2020

Uh oh!

elizabethandrews commented Dec 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

AaronBallman Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keryell commented Dec 10, 2020

Uh oh!

elizabethandrews commented Dec 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews commented Dec 18, 2020

Uh oh!

elizabethandrews commented Dec 18, 2020

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

romanovvlad commented Dec 21, 2020

Uh oh!

elizabethandrews commented Dec 21, 2020

Uh oh!

elizabethandrews commented Dec 21, 2020

Uh oh!

Uh oh!