[SYCL] Use aux triple when mangling neon vectors - #22877
Conversation
Changes in ItaniumMangle.cpp are to make this aux-triple and SYCL aware, which will cause us to go into aarch64 mangling path that correctly handles long long for windows. Checks are added to SemaType to ensure that the selected long or long long is indeed 64-bit, previously this would allow 32-bit long to be a valid polyvector type (which it's not). These are both tested with neon-polyvector-types.cpp. The -verify lines check for error for non 64-bit types, long on windows and int on linux. The -verify=quiet ensures that we use the aarch64 mangling path, the non aarch64 mangling path only accepts long long and not long. Co-authored-by: Lomuller, Victor <vlomuell@qti.qualcomm.com>
|
What was the diagnostic you were seeing before this change? @tahonermann can you take a look at this as well? |
tahonermann
left a comment
There was a problem hiding this comment.
I'm somewhat confused about what the root problem really is. As far as I can tell, NEON polynomial vectors can have base types that are 32-bit (according to ARM documentation) so long as the element count is correct.
The fix for mangling more or less makes sense, though I'm not sure why we would care that mangling of vector types is consistent across host and device compilation.
I noted a few other issues.
|
I don't see Vector data types ^
|
Perhaps that is a documentation mistake. That same doc includes both
I don't see any explicit indication that 32-bit poly types are not supported; most of the docs can be read as implying that they are. |
Sorry, ignore me, I clearly don't know what I'm talking about. When I looked further, other documents do make it clear that |
The original diagnostic was This could be reproduced by any file on windows (possibly also linux) including Now the current situation is that the error we ran into has been fixed in 48b7530, I hadn't seen these when posting this change. I'll remove the SYCL offloading changes from Sema and focus on the |
This is already taken care of by an earlier change to dpc++
tahonermann
left a comment
There was a problem hiding this comment.
Thanks, @frengels, this looks good to me.
|
@bader I see tests have not run for this PR, with the message "This workflow requires approval from a maintainer". I've never encountered this before. Can I approve the workflows? |
|
@intel/llvm-gatekeepers please consider merging |
@elizabethandrews, please, read https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories to understand the reasons for this repository workflows configuration.
If PR changes look reasonable, you can approve the workflow run. |
|
@frengels looks like the tests don't pass. Can you please take a look? There also appears to be a ClangFormat error. |
|
Fixed up the test. The issue was that due to the upstream change which fixed the original issue being made in a different location, it doesn't error even when using a bad poly type (32-bit). |
The aux triple needs to be passed for the mangling to go into the correct function.
Without this the
arm_neon.hheader will generate unsigned poly types as is donefor aarch64. But due to not checking the aux triple it will require the arm signed poly types.