[SYCL] Move the rest of attribute handling to SemaSYCL #15093
Conversation
This completes the work started by intel#14955.
frasercrmck
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
frasercrmck
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers this is ready for merging, thank you |
| break; | ||
| case ParsedAttr::AT_IntelReqdSubGroupSize: | ||
| handleIntelReqdSubGroupSize(S, D, AL); | ||
| S.SYCL().handleIntelReqdSubGroupSizeAttr(D, AL); |
There was a problem hiding this comment.
@frasercrmck, that's not exactly correct change. SYCL reqd_sub_group_size re-uses the attribute from OpenCL. Therefore, with this change we are enabling some extra/different handling for the attribute in OpenCL C mode.
This should be handled similar to work group size hint.
I suppose this is more if a #FYI, @dklochkov-emb encountered this in #15798 and I think that we will apply the fix there
There was a problem hiding this comment.
Oh yeah, sorry about that. I think the mistake was actually made earlier.
There was a problem hiding this comment.
It is a bit strange to me that we have different semantic handling for the openCL versions of these attributes vs SYCL spelling. Do we actually expect that to be the case?
I took a closer look at #15798 and at least for reqd_sub_group_size it looks like the semantic checks are pretty similar for both spellings. I see some additional warnings for NVPTX and AMD triples for SYCL spelling which aren't present for openCL handler but I do not know the functionality well enough to say whether the warnings are correct or not in openCL case.
#15798 crashes because the new handler added is not processing the argument correctly. As I commented in the PR, we don't need a new handler here. We can just use the old handler and modify the deprecation function.
For consistency with other attributes like work_group_size_hint, we could also call the openCL handler for non SYCL case but this is technically not necessary in this PR (although I am not against making the change now for consistency while we investigate). It is an orthogonal issue which we should investigate along with other similar attributes to check if the diversion is needed
There was a problem hiding this comment.
#15798 crashes because the new handler added is not processing the argument correctly. As I commented in the PR, we don't need a new handler here. We can just use the old handler and modify the deprecation function.
Yeah, we started with this, but along the way we though that maybe it is better to preserve the original handling for OpenCL attribute from the upstream simply to reduce amount of customizations.
For consistency with other attributes like work_group_size_hint, we could also call the openCL handler for non SYCL case but this is technically not necessary in this PR (although I am not against making the change now for consistency while we investigate). It is an orthogonal issue which we should investigate along with other similar attributes to check if the diversion is needed
It would be very good to hear a feedback from you, or anyone who is working on the upstreaming about that. What we do at intel/llvm should support the upstreaming effort and if we think that SYCL and OpenCL attributes should be separate entities (due to spelling and arguments handling: constant expressions vs literals), then I think it worth to explore the path with a separate handler. If not, then we can indeed just add a few if statements to the SYCL handler to account for OpenCL spelling (I remember that the main point of failure was lack of scope for the attribute which we unconditionally expect for SYCL).
There was a problem hiding this comment.
Differences in spelling should not necessitate different handlers. Unless the semantics of the attributes vary, IMO separate handlers are not required. However, it is unclear to me without investigation whether if/how the semantics vary and if they do for all attributes. And if some attributes vary in semantics maybe, it makes sense to have separate handlers for all to maintain consistency. I'll send an email to start a discussion on this with Aaron and Tom for their opinions here since they will have better background knowledge of similar cases in community clang.
5 participants
This completes the work started by #14955.