Skip to content

[SYCL] Disable diagnostics for neon attributes for SYCL device compilation - #22877

Open
frengels wants to merge 4 commits into
intel:syclfrom
frengels:neon-diagnostics
Open

[SYCL] Disable diagnostics for neon attributes for SYCL device compilation#22877
frengels wants to merge 4 commits into
intel:syclfrom
frengels:neon-diagnostics

Conversation

@frengels

@frengels frengels commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This would previously error out as the aux triple wasn't used for checking against neon vector types. Now the aux target is used when sycl is enabled for both sema and mangling.

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.

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>
@frengels
frengels requested a review from a team as a code owner August 5, 2026 14:56
@frengels frengels changed the title Disable diagnostics for neon attributes for SYCL device compilation [SYCL] Disable diagnostics for neon attributes for SYCL device compilation Aug 6, 2026
@elizabethandrews

Copy link
Copy Markdown
Contributor

What was the diagnostic you were seeing before this change? @tahonermann can you take a look at this as well?

@tahonermann tahonermann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread clang/lib/Sema/SemaType.cpp Outdated
Comment thread clang/test/SemaSYCL/neon-polyvector-types.cpp
Comment thread clang/lib/AST/ItaniumMangle.cpp
Comment thread clang/lib/Sema/SemaType.cpp Outdated
@frengels

frengels commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

I don't see poly32_t mentioned in the docs I'm referencing. https://arm-software.github.io/acle/main/acle.html#vector-data-types

Vector data types ^

The base types are ..., poly8_t, poly16_t, poly64_t, poly128_t, and ... .
Could you point me to where it mentions poly32_t as one of the types?

@tahonermann

Copy link
Copy Markdown
Contributor

Could you point me to where it mentions poly32_t as one of the types?

Perhaps that is a documentation mistake. That same doc includes both int32_t and uint32_t as base types. This doc states that intrinsics exist for 32-bit base types.

The Arm Neon architecture uses a 64-bit or 128-bit register file. In a 64-bit case, you can work with either eight 8-bit, four 16-bit, or two 32-bit elements. In a 128-bit case, you can work with either sixteen 8-bit, eight 16-bit, four 32-bit, or two 64-bit elements.
...
<type> is the data type (int, uint, float, or poly).
<size> is the number of bits used for the data type (8, 16, 32, 64).
<number of lanes> defines how many lanes.

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.

@tahonermann

Copy link
Copy Markdown
Contributor

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 poly32_t is not a thing.

@frengels

frengels commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

What was the diagnostic you were seeing before this change? @tahonermann can you take a look at this as well?

The original diagnostic was

C:\repos\intel-llvm\build\buildbot-native-Release\lib\clang\21\include\arm_neon.h:45:24: error: invalid vector element type 'poly8_t' (aka 'unsigned char')
   45 | typedef __attribute__((neon_polyvector_type(8))) poly8_t poly8x8_t;
      |                        ^

This could be reproduced by any file on windows (possibly also linux) including arm_neon.h. The aux target's defines would be enabled and therefore define poly8_t as uint8_t. But later during Sema in isPermittedNeonBaseType it no longer knows about being aarch64 causing it to require signed integers and resulting in that error.

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 ItaniumMangle.cpp changes, and test cases. The issue with ItaniumMangle.cpp being similar to the earlier however only occurring on linux. Again losing the target info causing to enter the wrong mangling function (arm vs aarch64).

@frengels
frengels requested a review from tahonermann August 11, 2026 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants