How should duplicate attributes be handled?

[AaronBallman]

We're inconsistent with how we report duplicate attributes and I'm wondering what direction we'd like to resolve the inconsistencies.

Some attributes check to see if the same attribute was already applied to the declaration, but do not check whether the arguments to the attributes are the same. We then issue a diagnostic saying that the attribute is applied with different parameters (which may be wrong!). e.g., https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3194

Some attributes check the attribute argument values to see if they're different between the two attributes and only diagnose if they're different. e.g., https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3180

Still others use a helper method that we introduced called checkForDuplicateAttribute(), which reports any kind of duplication regardless of arguments. We use this both for attributes that have no arguments (https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L5597) and attributes that have arguments (https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L5621).

In community, the usual approach is to only diagnose a duplicate attribute if the arguments are different between the attributes, and to warn + retain (https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L2980) or to warn + drop the new attribute (https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L3026, https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclAttr.cpp#L3227).

Given that we seem to almost uniformly warn + retain (except for https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaDeclAttr.cpp#L3295), I think the correct fix here is to not warn about duplicate attributes that have no arguments or when the attribute arguments are identical (which means we'll silently accumulate the duplicates in the AST). For attributes whose arguments differ, it may make sense to continue to diagnose the confusion, but should we drop one attribute or the other (or both)?

[MrSidims]

I'm down with only diagnosing duplicated attributes with mismatching arguments. Regarding whether we shall ignore duplicated attribute or we shall pick one of them. Actually, I slightly lean towards replacing warning diagnostic with an error, in some cases it might be more user-friendly, what do you think?

[AaronBallman]

Okay, I can go with only warning on actually mismatched arguments. I was a bit worried that this may kick some diagnostics back to template instantiation time, but now that I think on it... that would require some contortions to get duplicate attributes with different arguments on the redeclarations. :-)

I'm not keen on making it an error -- the code is certainly confused, but picking one attribute or the other and going with it is a very reasonable recovery strategy that can be applied consistently (always pick first or always pick last). To me, that makes this more of a warning than an error, but it really depends on the specific attribute. If "we guessed wrong and now your program is insecure" is the outcome, then an error seems better. If "we guessed wrong and now your program is slightly less efficient than it could have been had we picked the other way", then a warning seems pretty reasonable. I don't know the domain well enough to make that call for all of these attributes, but they don't /seem/ likely to cause security issues in this case. WDYT?

[AaronBallman]

A related question -- I assume we want to warn on duplicate attributes in redeclarations as well?

__attribute__((intel_reqd_sub_group_size(4), intel_reqd_sub_group_size(5)) void func(void); // We diagnose this.

__attribute__((intel_reqd_sub_group_size(4)) void func(void);
__attribute__((intel_reqd_sub_group_size(5)) void func(void) {} // We don't diagnose this.

[AlexeySachkov]

Tagging @Pennycook, @mkinsner, @GarveyJoe and @bashbaug for additional inputs here

[GarveyJoe]

I like the idea of only providing a diagnostic if the attributes have mismatching arguments. There is one exception I can think of though: ivdep. If I have [intel::ivdep(N)] and [intel::ivdep(M)] on my loop then regardless of the order they are applied there is a clear way to handle this situation: take whichever is larger between N and M as the larger one provides strictly more information than the smaller. I don't think we need to emit a diagnostic in that case.

[AaronBallman]

I like the idea of only providing a diagnostic if the attributes have mismatching arguments. There is one exception I can think of though: ivdep. If I have [intel::ivdep(N)] and [intel::ivdep(M)] on my loop then regardless of the order they are applied there is a clear way to handle this situation: take whichever is larger between N and M as the larger one provides strictly more information than the smaller. I don't think we need to emit a diagnostic in that case.

I thought about this a bit over the weekend and have a different idea. I think the rule of thumb we should follow is: for attributes that can be duplicated through redeclarations, I think we should warn only if the attribute arguments differ. For attributes that can't be duplicated through redeclarations (like statement attributes such as ivdep), I think we should diagnose always regardless of values.

I think this is useful because redeclarations can be easy to trigger while still getting the same attribute. e.g.,

[[intel::reqd_sub_group_size(4)]] void func();
[[intel::reqd_sub_group_size(4)]] void func() {} // Would be silly to diagnose this

[[intel::reqd_sub_group_size(4)]] void func();
[[intel::reqd_sub_group_size(5)]] void func() {} // Would make sense to diagnose this

However, the same is definitely not true for things like statement attributes. The only way to run into that situation would be:

[[intel::ivdep(4), intel::ivdep(4)]] for (...) {}
[[intel::ivdep(4)]] [[intel::ivdep(5)]] for (...) {}

I can easily envision users writing the redeclaration case code, but I can't imagine a user who adds duplicate statement attributes by accident. WDYT?

[GarveyJoe]

It's probably rare for a user to specify ivdeps with different arguments if they are both literals, but I could see it happening if one or both are macros set elsewhere in the code. Maybe they both come from different properties of the code but which one is larger (and thus meaningful) depends on the values of the macros which the user may change from one compile to the next.

[AaronBallman]

I don't see how you'd ever get two attributes with different values on the same statement with any sane use of macros, but I can live with the behavior of not diagnosing in this situation, too. The reason I was pushing back slightly is because any time we have to check the values explicitly, we have to add the same checking code to the template instantiation case (more code to maintain, slower template instantiations). If we bailed on the mere presence of the duplicate attribute, we'd not have to add extra complexity to the template instantiation logic. That said, I think we've got a defensible path forward here that's far more consistent than the status quo. Thanks for the discussion!

[KornevNikita]

Hi! There have been no updates for at least the last 60 days, though the ticket has assignee(s).

@AaronBallman, could I ask you to take one of the following actions? :)

• Please provide an update if you have any (or just a small comment if you don't have any yet).
• OR mark this issue with the 'confirmed' label if you have confirmed the problem/request and our team should work on it.
• OR close the issue if it has been resolved.
• OR take any other suitable action.

Thanks!

[AaronBallman]

I think the consensus on the issue is that we should do something here. I removed myself as Assignee because I won't have the bandwidth to work on this myself.