[SYCL] Turn indirectly-callable property into sycl_device attribute

[AlexeySachkov]

This PR is a part of virtual functions feature being designed in #10540. One aspects of the proposed language specification is to be able to mark functions as device functions using SYCL compile-time properties mechanism.

Compile-time properties are lowered into
[[__sycl_detail__::add_ir_attribute_function()]] attribute and this patch introduces extra processing for the attribute: property names passed to the attribute are analyzed so we can generate other attributes based on those strings.

The only example of such property/attribute pair is indirectly-callable string that indicates that a function with such property should be treated as if it had sycl_device attribute attached to it and we do exactly that implicitly now.

Note that there were changes required to the attribute parsing logic: because we now access its argument way earlier than we used to, that logic had to be improved to be more robust and pass existing SemaSYCL/attr-add-ir-attributes.cpp test.

[AlexeySachkov]

Note for reviewers: this particular change was done to avoid assertion failure within APInt on the following test case:

llvm/clang/test/SemaSYCL/attr-add-ir-attributes.cpp

Line 58 in c02a98c

[[__sycl_detail__::add_ir_attributes_function("Attr1", CEStr)]] void FunctionCEVal2() {}

[AlexeySachkov]

Note for reviewers: this particular change was done to avoid assertion failure due to invalid attribute name (nullopt being returned) on the following test case:

llvm/clang/test/SemaSYCL/attr-add-ir-attributes.cpp

Line 78 in c02a98c

[[__sycl_detail__::add_ir_attributes_function(CEAttrName1, nullptr)]] void FunctionCEName1() {}

[elizabethandrews]

I'm a little confused. Why did your change cause this and the test mentioned below to break? I understand getFilteredAttributeNameValuePairs is now called earlier but I don't understand why that causes these specific issues.

[AlexeySachkov]

I'm a little confused. Why did your change cause this and the test mentioned below to break?

I honestly don't understand that myself. I can do a bit more of a debugging here to see why we see this particular AST and not another. My current guess is that it can be related to evaluation of constexpr variables, i.e. by the time we parse this attribute, we haven't done some processing of CEAttrName1 (due to lazy nature of that processing), but it is always done before we reach CodeGen and therefore we haven't seen this problem before

[AlexeySachkov]

Ok, I seem to have figured it out. The issue is reproducible even without my changes, it is just hidden by the test without my changes. If you try to compile the following file with plain intel/llvm:

$ cat p.cpp
constexpr const char CEAttrName1[] = "CEAttr1";

__attribute__((sycl_device)) [[__sycl_detail__::add_ir_attributes_function(CEAttrName1, nullptr)]] void FunctionCEName1() {}

$ clang++ -cc1 -fsycl-is-device -fsyntax-only p.cpp
clang++: github.com/intel/llvm/build/tools/clang/include/clang/AST/Attrs.inc:11988: llvm::SmallVector<std::pair<std::__cxx11::basic_strin
g<char>, std::__cxx11::basic_string<char> >, 4> clang::SYCLAddIRAttributesFunctionAttr::getFilteredAttributeNameValuePairs(const std::optional<llvm::SmallSet<
llvm::StringRef, 4> >&, const clang::ASTContext&) const: Assertion `NameStr && "Attribute name is not a valid string."' failed.
PLEASE submit a bug report to https://github.com/intel/llvm/issues and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: ./bin/clang++ -cc1 -fsycl-is-device -fsyntax-only p.cpp
1.      <eof> parser at end of file
2.      p.cpp:3:123: parsing function body 'FunctionCEName1'

Because the test also contains lines that cause compilation errors, we exit early from parsing/handling some of the things and therefore we don't see this assert. With the attribute arguments analysis performed earlier, we always go through that path and any "early exits" due to errors in the code do not help us avoid this issue.

[premanandrao]

LGTM

[smanna12]

LGTM. Thank you for applying the comment.

[AlexeySachkov]

Pre-commit on CUDA seem to have failed due to infrastructure reason. Previous run had passed and the only change added since then is a non-functional commit to use '\0' instead of 0 when comparing char literals. I will proceed with merge

[AlexeySachkov]

@rdeodhar, FYI: you can probably re-use this infrastructure in free kernel functions extension to avoid the need of using SYCL_EXTERNAL when defining kernel as a plain function

[rdeodhar]

@rdeodhar, FYI: you can probably re-use this infrastructure in free kernel functions extension to avoid the need of using SYCL_EXTERNAL when defining kernel as a plain function

I noticed that some joint matrix tests used add_ir_attributes_function with non-constexpr values for the value part. Then, using getFilteredAttributeNameValuePairs as the primary means of detecting free functions choked over those joint matrix functions. Using SYCL_EXTERNAL as a first filter eliminated those joint matrix functions from being run through getFilteredAttributeNameValuePairs. That is why free functins currently use SYCL_EXTERNAL as the first criterion.

[AlexeySachkov]

I noticed that some joint matrix tests used add_ir_attributes_function with non-constexpr values for the value part.

Curious. My understanding is that the attribute is intended to generate LLVM IR, we can't pass anything that can't be evaluated in compile-time to it. Perhaps those tests were hitting cases that I've fixed in this PR with the attribute arguments parsing

[rdeodhar]

Yes, those values should be constexprs. I plan to try again with your fix in place to see if that issue no longer affects free function detection. Otherwise, I will work with the owners of those joint matrix tests to make everything constexpr.