[SYCL][clang] Fix more free-function kernel integration header cases #20877
Conversation
This commit fixes a number of known issues when the integration header generates prototypes of the free-function kernels. These changes focus on the additional removal of aliasing and proper handling of templated template arguments. This commit also adds disabled test cases for a known issue with unresolved nested templated type aliases. These are cases for future work. Signed-off-by: Larsen, Steffen <[email protected]> Co-authored-by: Sachkov, Alexey <[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
steffenlarsen
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Fznamznon
left a comment
Fznamznon
left a comment
There was a problem hiding this comment.
Just questions, will continue with meaningful review on Monday.
@elizabethandrews , @premanandrao given the size of the pr you're weclome to review as well.
| } | ||
|
|
||
| const TemplateSpecializationType * | ||
| getAsTemplateSpecializationTypeWithoutAliases(const ASTContext &Ctx) const; |
There was a problem hiding this comment.
Is there any reason to have it as a member of Type class rather than a static function in SemaSYCL.cpp?
| const TemplateSpecializationType *TST = | ||
| getAsNonAliasTemplateSpecializationType(); | ||
| if (!TST) | ||
| return TST; |
There was a problem hiding this comment.
That looks a bit unusual, can we make it explicit?
| return TST; | |
| return nullptr; |
| return TST; | ||
| } | ||
|
|
||
| const TemplateSpecializationType * |
There was a problem hiding this comment.
Can we also have a documentation comment explaining what the function does and what is the difference from getAsNonAliasTemplateSpecializationType ?
|
|
||
| const TemplateSpecializationType *TSTAsNonAlias = | ||
| TST->getAsNonAliasTemplateSpecializationType(); | ||
| TST->getAsTemplateSpecializationTypeWithoutAliases(Context); |
There was a problem hiding this comment.
So I was kind of expecting that the visitor soluiton would help us to avoid unwrapping and modifying types manually like getAsTemplateSpecializationTypeWithoutAliases does. Is that not the case? Could you please elaborate why do we need such a complex solution?
| /// What this visitor does is it iterates over both in sync, picking the right | ||
| /// values from one or another. | ||
| /// | ||
| /// Moral of the story: drop integration header ASAP (but that is blocked |
There was a problem hiding this comment.
Lol, now we have two comments in SemaSYCL saying drop integration header.
2 participants
This commit fixes a number of known issues when the integration header generates prototypes of the free-function kernels. These changes focus on the additional removal of aliasing and proper handling of templated template arguments.
This commit also adds disabled test cases for a known issue with unresolved nested templated type aliases. These are cases for future work.