[SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes #3251

smanna12 · 2021-02-23T14:34:30Z

This patch

refactors two declaration attributes using [SYCL] Refactor the way we handle duplicate attribute logic #3224:
[[intel::private_copies()] and [[intel::max_replicates()]] to better fit for community standards.
refactors the way we handle duplicate attributes and
mutually exclusive attributes logic when present on a given declaration.
handles redeclarations or template instantiations properly.
adds test

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

…ates()]] attributes This patch 1. refactors two declaration attributes: [[intel::private_copies()] and [[intel::max_replicates()]] using intel#3224 as a template to better fit for community standards. 2. refactors the way we handle duplicate attributes and mutually exclusive attributes logic when present on a given declaration. 3. handles redeclarations or template instantiations properly. 4. adds test Signed-off-by: Soumi Manna <[email protected]>

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

smanna12 · 2021-02-23T15:43:20Z

The patch will be rebased after #3234 lands.

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

clang/lib/Sema/SemaDeclAttr.cpp

clang/include/clang/Sema/Sema.h

clang/lib/Sema/SemaDeclAttr.cpp

smanna12 · 2021-02-26T14:19:30Z

Thanks @AaronBallman for feedbacks. I will address them and submit new patch.

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

smanna12 · 2021-03-06T04:19:28Z

clang/lib/Sema/SemaDeclAttr.cpp

+  // [[intel::max_replicates()]] and [[intel::fpga_register]]
+  // attributes are incompatible.
+  if (const auto *DeclAttr = D->getAttr<IntelFPGARegisterAttr>()) {
+    Diag(DeclAttr->getLoc(), diag::err_attributes_are_not_compatible)
+        << &A << DeclAttr;
+    Diag(A.getLoc(), diag::note_conflicting_attribute);
+    return nullptr;
+  }


I did not add checkAttrMutualExclusion<> here since this will generate incompatible attribute on first line instead of second line

// Merging of incompatible args
[[intel::max_replicates(12)]] extern const int var_max_replicates_2;
[[intel::fpga_register]] const int var_max_replicates_2 = 0;

Hmm, the other merge*Attr use checkAttrMutualExclusion(), so I don't think the order is incorrect.

D holds the attributes we've already applied, so if D has an IntelFPGARegisterAttr, then the conflict we want to report is not with that attribute, but with the one we're about to add (IntelFPGAMaxReplicatesAttr). From that perspective, I think your code is diagnosing the opposite of what I'd expect -- it is diagnosing the IntelFPGARegisterAttr attribute location as being incompatible, but that's not the incompatible attribute -- the attempt to merge in IntelFPGAMaxReplicatesAttr is.

If you still think the behavior is wrong when using checkAttrMutualExclusion<>, can you post the exact diagnostic output you're getting from it?

@AaronBallman, thanks for the feedback.

I think your code is diagnosing the opposite of what I'd expect -- it is diagnosing the IntelFPGARegisterAttr attribute location as being incompatible,

Yes, this is what happening here.

I confused myself with CheckMutualExclusionSYCLLoopAttribute() that the incompatible diagnostic will be on second line where we have IntelFPGARegisterAttr attribute on declaration.

This is the output from checkAttrMutualExclusion<> with the current patch:

// Merging of incompatible attributes
//expected-error@+2{{'max_replicates' and 'fpga_register' attributes are not compatible}}
//expected-note@+2{{conflicting attribute is here}}
[[intel::max_replicates(12)]] extern const int var_max_replicates_2;
[[intel::fpga_register]] const int var_max_replicates_2 =0;

That order looks wrong to me. I'd expect:

[[intel::max_replicates(12)]] extern const int var_max_replicates_2; // expected-note {{conflicting attribute is here}} [[intel::fpga_register]] const int var_max_replicates_2 =0; // expected-error {{'max_replicates' and 'fpga_register' attributes are not compatible}}

One question I have is: are you sure this behavior isn't coming from checkIntelFPGARegisterAttrCompatibility() rather than this change?

That order looks wrong to me. I'd expect:

[[intel::max_replicates(12)]] extern const int var_max_replicates_2; // expected-note {{conflicting attribute is here}} [[intel::fpga_register]] const int var_max_replicates_2 =0; // expected-error {{'max_replicates' and 'fpga_register' attributes are not compatible}}

This is what i had before checkAttrMutualExclusion<>

// attributes are incompatible. if (const auto *DeclAttr = D->getAttr<IntelFPGARegisterAttr>()) { Diag(DeclAttr->getLoc(), diag::err_attributes_are_not_compatible) << &A << DeclAttr; Diag(A.getLoc(), diag::note_conflicting_attribute); return nullptr; } checkAttrMutualExclusion<> function changes the order because of this: template <typename AttrTy> static bool checkAttrMutualExclusion(Sema &S, Decl *D, const Attr &AL) { if (const auto *A = D->getAttr<AttrTy>()) { S.Diag(AL.getLocation(), diag::err_attributes_are_not_compatible) << &AL << A; S.Diag(A->getLocation(), diag::note_conflicting_attribute); return true; } return false; }

Thanks for checking! I don't want us to get too hung up on this, but something fishy is going on that I'm not quite understanding. Your code looks like it should issue the diagnostics in the wrong order -- you check to see if the declaration has an existing IntelFPGARegisterAttr attribute and if it does, it issues the diagnostic on that previous attribute (not the one being added). It then notes the one being added as being the conflicting attribute. This is backwards -- the initial attribute is fine (there were no attributes on the declaration for it to conflict with), it's the second attribute location that should be diagnosed as the error (rather than the note).

Are all of the merge*Attr() functions which call checkAttrMutualExclusion getting the order wrong? (I suppose that's a possibility I hadn't considered.) Or is it perhaps because we don't have any merge function for IntelFPGARegisterAttr when we should have one

Thanks to some excellent detective work from @smanna12, we figured out that this is a community issue. All of the merge functions currently calling checkAttrMutualExclusion() have the same backwards diagnostic behavior. Yay for getting to the bottom of the mystery!

I think you should use checkAttrMutualExclusion() and add a FIXME comment to the test about this behavior not being ideal, but it being an issue that should be solved in community. I don't think we need to rush to get a fix into community because the diagnostic behavior here is sufficient for users to understand what's wrong with their code, even if the diagnostic location isn't ideal (redeclaration merging is a bit of an edge case for attribute handling).

Thanks @AaronBallman for the comments and the detailed explanation.

I have added FIXME comments in the test.

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

premanandrao · 2021-03-08T19:01:00Z

clang/lib/Sema/SemaDeclAttr.cpp

  S.CheckDeprecatedSYCLAttributeSpelling(A);

-  S.addIntelSingleArgAttr<IntelFPGAMaxReplicatesAttr>(D, A, A.getArgAsExpr(0));
+  Expr *E = A.getArgAsExpr(0);
+  S.AddIntelFPGAMaxReplicatesAttr(D, A, E);


Suggested change

S.AddIntelFPGAMaxReplicatesAttr(D, A, E);

S.AddIntelFPGAMaxReplicatesAttr(D, A, A.getArgAsExpr(0));

premanandrao · 2021-03-08T19:03:31Z

clang/lib/Sema/SemaDeclAttr.cpp

  S.CheckDeprecatedSYCLAttributeSpelling(A);

-  S.addIntelSingleArgAttr<IntelFPGAPrivateCopiesAttr>(D, A, A.getArgAsExpr(0));
+  Expr *E = A.getArgAsExpr(0);
+  S.AddIntelFPGAPrivateCopiesAttr(D, A, E);


Suggested change

S.AddIntelFPGAPrivateCopiesAttr(D, A, E);

S.AddIntelFPGAPrivateCopiesAttr(D, A, A.getArgAsExpr(0));

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

AaronBallman

LGTM!

smanna12 · 2021-03-09T13:49:36Z

Thanks @AaronBallman.

smanna12 · 2021-03-09T13:54:27Z

@premanandrao / @elizabethandrews , can you please review this? Thanks.

elizabethandrews

Just a few nits

clang/test/SemaSYCL/intel-fpga-local.cpp

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

clang/test/SemaSYCL/sycl-device-intel-fpga-local.cpp

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

elizabethandrews

LGTM Thanks!

clang/test/SemaSYCL/intel-fpga-local.cpp

smanna12 added 2 commits February 23, 2021 06:18

Merge remote-tracking branch 'my_remote/sycl' into RefactorAttr

11d49ae

smanna12 closed this Feb 23, 2021

smanna12 reopened this Feb 23, 2021

Fix conflicts

9682b76

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

smanna12 changed the title ~~[SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replic…~~ [SYCL] [WIP] Refactor of [[intel::private_copies()] and [[intel::max_replic… Feb 23, 2021

smanna12 changed the title ~~[SYCL] [WIP] Refactor of [[intel::private_copies()] and [[intel::max_replic…~~ [SYCL] [WIP] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes Feb 23, 2021

Fix format issues

52309f9

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

smanna12 changed the title ~~[SYCL] [WIP] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes~~ [SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes Feb 23, 2021

smanna12 changed the title ~~[SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes~~ [SYCL] [WIP] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes Feb 23, 2021

Fix errors

1a75787

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

smanna12 changed the title ~~[SYCL] [WIP] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes~~ [SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes Feb 23, 2021

Merge remote-tracking branch 'my_remote/sycl' into RefactorAttr

a71c5b0

smanna12 marked this pull request as ready for review February 25, 2021 14:21

smanna12 requested review from elizabethandrews and premanandrao as code owners February 25, 2021 14:21

smanna12 requested a review from AaronBallman February 25, 2021 14:21

AaronBallman reviewed Feb 25, 2021

View reviewed changes

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

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

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

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

smanna12 added 7 commits March 4, 2021 15:49

Address review comments

d0551b6

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

Merge remote-tracking branch 'my_remote/sycl' into RefactorAttr

352284e

Fix error

b674671

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

Address review comments

88cebce

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

Address review comments

3eb930a

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

Fix format issues

b803d54

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

Add comments for each added tests

32e5232

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

smanna12 requested a review from AaronBallman March 6, 2021 04:13

smanna12 commented Mar 6, 2021

View reviewed changes

Merge remote-tracking branch 'my_remote/sycl' into RefactorAttr

dc507c3

smanna12 added 3 commits March 7, 2021 10:45

Update comments in test

a98f311

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

Update comment in test

179852f

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

Fix Diagnostic

90f8dd5

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

premanandrao reviewed Mar 8, 2021

View reviewed changes

Add FIXME comments and address review comments

6548d63

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

smanna12 requested review from premanandrao and removed request for AaronBallman March 8, 2021 21:25

Update test comments

80d2f75

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

AaronBallman previously approved these changes Mar 9, 2021

View reviewed changes

elizabethandrews reviewed Mar 9, 2021

View reviewed changes

clang/test/SemaSYCL/intel-fpga-local.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/intel-fpga-local.cpp Outdated Show resolved Hide resolved

address review comments

6a6470e

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

smanna12 dismissed AaronBallman’s stale review via 6a6470e March 9, 2021 14:38

smanna12 requested a review from elizabethandrews March 9, 2021 14:41

elizabethandrews reviewed Mar 9, 2021

View reviewed changes

clang/test/SemaSYCL/sycl-device-intel-fpga-local.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/sycl-device-intel-fpga-local.cpp Outdated Show resolved Hide resolved

Address review comments

ece75b5

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

smanna12 requested a review from elizabethandrews March 9, 2021 20:02

elizabethandrews approved these changes Mar 10, 2021

View reviewed changes

elizabethandrews reviewed Mar 10, 2021

View reviewed changes

clang/test/SemaSYCL/intel-fpga-local.cpp Show resolved Hide resolved

smanna12 mentioned this pull request Mar 10, 2021

Ignore duplicate attribute when adding or merging. #3336

Closed

bader merged commit f32c431 into intel:sycl Mar 10, 2021

smanna12 mentioned this pull request Mar 10, 2021

[SYCL][FPGA] Refactor of [[intel::max_global_work_dim()]] attribute #3326

Merged

	S.AddIntelFPGAMaxReplicatesAttr(D, A, E);
	S.AddIntelFPGAMaxReplicatesAttr(D, A, A.getArgAsExpr(0));

	S.AddIntelFPGAPrivateCopiesAttr(D, A, E);
	S.AddIntelFPGAPrivateCopiesAttr(D, A, A.getArgAsExpr(0));

[SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes #3251

[SYCL] Refactor of [[intel::private_copies()] and [[intel::max_replicates()]] attributes #3251

Uh oh!

Conversation

smanna12 commented Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smanna12 commented Feb 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smanna12 commented Feb 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 commented Mar 9, 2021

Uh oh!

smanna12 commented Mar 9, 2021

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smanna12 commented Feb 23, 2021 •

edited

Loading

smanna12 Mar 8, 2021 •

edited

Loading

smanna12 Mar 8, 2021 •

edited

Loading