-
Notifications
You must be signed in to change notification settings - Fork 778
[SYCL][Clang] Add ARM64_SPIRV64 and ARM64SPIR64 target info #18859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello frens @intel/llvm-reviewers-cuda |
83bf4de
to
41e1f96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good, but I wonder if this could benefit upstream LLVM as well. Seems like at least some of it is independent of SYCL.
I'm not 100% sure there is the basic infrastructure yet for this. But I think it would be nice to stop creating these hardcoded subvariants (it'll probably not fly well upstream anyway), they are hacks and it doesn't scale. E.g. AMD has one TargetInfo and NV has 2 (for the 32 and 64 variants) and that enough to support all host... We only need the SPIRTargetInfo, SPIR64TargetInfo, SPIRV32TargetInfo and SPIRV64TargetInfo variants and a proper implementation of |
So for now do you think it's OK to merge this patch and add a TODO for upstreaming? Looks like some refactoring is needed in order to get something that would be agreeable to upstream. |
Instead of asserting false, report a fatal error if the user provided triple is MSVC and not ARM64 or x64_64 when using SPIR64 or SPIRV64 device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for now do you think it's OK to merge this patch and add a TODO for upstreaming? Looks like some refactoring is needed in order to get something that would be agreeable to upstream.
That would work for me if you also create an issue :) Assign that to me and I'll dispatch that to the FE looking after the upstreaming.
_M_ARM64 will be defined by the base getTargetDefines instance. Add a test to verify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @Naghasan ! Ping @intel/dpcpp-cfe-reviewers |
Ping @intel/llvm-gatekeepers this is good to go 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authorship inconsistent
Add TargetInfo for MSVC ARM64 host with SPIR64 or SPIRV64 device target.