[SYCL][clang] Implement merging of add_ir_attributes_* attributes #20419
Conversation
This commit adds the implementation logic for merging __sycl_detail__::add_ir_attributes_* attributes, which will enable the ability to apply multiple of the same attribute to the same declaration/definition, as long as the values associated to the same names in the attributes do not conflict. This is needed for SYCL free function support as the way SYCL properties are applied are through macros expanding to __sycl_detail__::add_ir_attributes_* with a single name-value-pair. Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| [[__sycl_detail__::add_ir_attributes_function({"Attr1", "Attr3"}, "Attr2", "Attr1", true, 1)]] void FunctionRedecl3(); | ||
| [[__sycl_detail__::add_ir_attributes_function({"Attr3", "Attr1"}, "Attr1", "Attr2", 1, true)]] void FunctionRedecl3(); // expected-note {{conflicting attribute is here}} | ||
| [[__sycl_detail__::add_ir_attributes_function({"Attr3", "Attr1"}, "Attr1", "Attr2", 1, true)]] void FunctionRedecl3(); | ||
| [[__sycl_detail__::add_ir_attributes_function({"Attr1", "Attr3"}, "Attr1", "Attr2", 1, false)]] void FunctionRedecl3(); // expected-error {{attribute '__sycl_detail__::add_ir_attributes_function' is already applied with different arguments}} expected-note {{conflicting attribute is here}} |
There was a problem hiding this comment.
Sorry what is the conflict here? I might just be misunderstanding how filters work, but you said only the attributes in the filter list are applied, which means only Attr1 is applied right? Isn't it's value 1 in all cases?
There was a problem hiding this comment.
We resolve conflicts without applying the filter. The argument is that from clang's point-of-view, the values are still conflicting, even if they don't end up producing IR attributes. I don't think this behavior is necessarily strictly needed, but I like to think it fits the mental model.
There was a problem hiding this comment.
Hmm....functionally that is questionable to me. If filters determine what the attributes are why should we care if non-applied attributes conflict?
There was a problem hiding this comment.
The problem with applying the filters when merging is that we lose the information about the values before we would normally apply the filters. We don't currently use the filters all that much, but I also don't see a use-case for us where we would want to allow conflicting values, even for the ones we filter out.
There was a problem hiding this comment.
Ok. I defer to you since I am not very familiar with the usage/functionality of these attributes. Please add AST tests though and maybe a comment somewhere how filters work in this context. It is not intuitive.
There was a problem hiding this comment.
The new tests are in 7c7173d
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Signed-off-by: Larsen, Steffen <[email protected]>
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
steffenlarsen
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers please consider merging |
2 participants
This commit adds the implementation logic for merging sycl_detail::add_ir_attributes_* attributes, which will enable the ability to apply multiple of the same attribute to the same declaration/definition, as long as the values associated to the same names in the attributes do not conflict.
This is needed for SYCL free function support as the way SYCL properties are applied are through macros expanding to
sycl_detail::add_ir_attributes_* with a single name-value-pair.