[clang] [Headers] Don't use unreserved name in avx512fp16intrin.h #98478

mstorsjo · 2024-07-11T13:17:13Z

This can cause breakage with user code that does "#define A ...".

llvmbot · 2024-07-11T13:17:44Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Martin Storsjö (mstorsjo)

Changes

This can cause breakage with user code that does "#define A ...".

Full diff: https://github.com/llvm/llvm-project/pull/98478.diff

1 Files Affected:

(modified) clang/lib/Headers/avx512fp16intrin.h (+36-36)

diff --git a/clang/lib/Headers/avx512fp16intrin.h b/clang/lib/Headers/avx512fp16intrin.h
index 4123f10c39513..3e11795757f98 100644
--- a/clang/lib/Headers/avx512fp16intrin.h
+++ b/clang/lib/Headers/avx512fp16intrin.h
@@ -282,75 +282,75 @@ _mm512_zextph256_ph512(__m256h __a) {
 #define _mm_comi_sh(A, B, pred)                                                \
   _mm_comi_round_sh((A), (B), (pred), _MM_FROUND_CUR_DIRECTION)
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comieq_sh(__m128h A,
-                                                          __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_EQ_OS,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comieq_sh(__m128h __A,
+                                                          __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_EQ_OS,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comilt_sh(__m128h A,
-                                                          __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_LT_OS,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comilt_sh(__m128h __A,
+                                                          __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_LT_OS,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comile_sh(__m128h A,
-                                                          __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_LE_OS,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comile_sh(__m128h __A,
+                                                          __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_LE_OS,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comigt_sh(__m128h A,
-                                                          __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_GT_OS,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comigt_sh(__m128h __A,
+                                                          __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_GT_OS,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comige_sh(__m128h A,
-                                                          __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_GE_OS,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comige_sh(__m128h __A,
+                                                          __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_GE_OS,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comineq_sh(__m128h A,
-                                                           __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_NEQ_US,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_comineq_sh(__m128h __A,
+                                                           __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_NEQ_US,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomieq_sh(__m128h A,
-                                                           __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_EQ_OQ,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomieq_sh(__m128h __A,
+                                                           __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_EQ_OQ,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomilt_sh(__m128h A,
-                                                           __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_LT_OQ,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomilt_sh(__m128h __A,
+                                                           __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_LT_OQ,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomile_sh(__m128h A,
-                                                           __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_LE_OQ,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomile_sh(__m128h __A,
+                                                           __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_LE_OQ,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomigt_sh(__m128h A,
-                                                           __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_GT_OQ,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomigt_sh(__m128h __A,
+                                                           __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_GT_OQ,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomige_sh(__m128h A,
-                                                           __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_GE_OQ,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomige_sh(__m128h __A,
+                                                           __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_GE_OQ,
                                 _MM_FROUND_CUR_DIRECTION);
 }
 
-static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomineq_sh(__m128h A,
-                                                            __m128h B) {
-  return __builtin_ia32_vcomish((__v8hf)A, (__v8hf)B, _CMP_NEQ_UQ,
+static __inline__ int __DEFAULT_FN_ATTRS128 _mm_ucomineq_sh(__m128h __A,
+                                                            __m128h __B) {
+  return __builtin_ia32_vcomish((__v8hf)__A, (__v8hf)__B, _CMP_NEQ_UQ,
                                 _MM_FROUND_CUR_DIRECTION);
 }

KanRobert · 2024-07-11T13:25:52Z

Hmm, what's your usage?

#define A

before

#include <avx512fp16intrin.h>

?

mstorsjo · 2024-07-11T13:33:15Z

Hmm, what's your usage?

#define A

before

#include <avx512fp16intrin.h>

?

Yes, exactly

AaronBallman

The only other unreserved name I could find in the file is:

llvm-project/clang/lib/Headers/avx512fp16intrin.h

Line 99 in d9c26b9

_mm512_set1_pch(_Float16 _Complex h) {

(uses h)

so we might as well fix this at the same time. We should probably have tests using https://clang.llvm.org/extra/clang-tidy/checks/bugprone/reserved-identifier.html that runs over all of the system headers we produce to issue a diagnostic when we use an unreserved identifier so that we catch this issue more explicitly.

mstorsjo · 2024-07-11T14:42:49Z

The only other unreserved name I could find in the file is:

llvm-project/clang/lib/Headers/avx512fp16intrin.h

Line 99 in d9c26b9

_mm512_set1_pch(_Float16 _Complex h) {

(uses h)
so we might as well fix this at the same time.

Done, thanks!

We should probably have tests using https://clang.llvm.org/extra/clang-tidy/checks/bugprone/reserved-identifier.html that runs over all of the system headers we produce to issue a diagnostic when we use an unreserved identifier so that we catch this issue more explicitly.

Oh, good to know that we do have a way to check these things automatically! Yeah, setting up such tests would indeed be good.

KanRobert

LGTM

AaronBallman

LGTM!

…lvm#98478) This can cause breakage with user code that does "#define A ...".

mstorsjo · 2025-10-03T08:15:19Z

The only other unreserved name I could find in the file is:

llvm-project/clang/lib/Headers/avx512fp16intrin.h

Line 99 in d9c26b9

_mm512_set1_pch(_Float16 _Complex h) {

(uses h)
so we might as well fix this at the same time. We should probably have tests using https://clang.llvm.org/extra/clang-tidy/checks/bugprone/reserved-identifier.html that runs over all of the system headers we produce to issue a diagnostic when we use an unreserved identifier so that we catch this issue more explicitly.

We seem to have gotten more cases of such unreserved identifiers in 83ad644 (see the fallout of it in #161736), so I think it's somewhat urgent to set up such automated testing of the Clang headers.

RKSimon · 2025-10-03T08:24:05Z

We already have header test coverage, and clang\test\Headers\x86-intrinsics-headers-clean.cpp in particular is there to catch this kind of thing:

https://github.com/RKSimon/llvm-project/blob/7eb5c08ac3a5bc524a5fe4e2e91db3a5b1ffe3cd/clang/test/Headers/x86-intrinsics-headers-clean.cpp#L7-L9

Are there any other warnings that we can add to help catch this?

mstorsjo · 2025-10-03T08:42:27Z

We already have header test coverage, and clang\test\Headers\x86-intrinsics-headers-clean.cpp in particular is there to catch this kind of thing:

https://github.com/RKSimon/llvm-project/blob/7eb5c08ac3a5bc524a5fe4e2e91db3a5b1ffe3cd/clang/test/Headers/x86-intrinsics-headers-clean.cpp#L7-L9

Are there any other warnings that we can add to help catch this?

I'm not sure if there are warning in Clang proper; @AaronBallman 's comment above suggested using https://clang.llvm.org/extra/clang-tidy/checks/bugprone/reserved-identifier.html. The libcxx testcase, https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/system_reserved_names.gen.py, uses a bunch of defines of every known potentially problematic symbol name, before testing including the header - e.g. like this:

#define SYSTEM_RESERVED_NAME This name should not be used in libc++
#define A SYSTEM_RESERVED_NAME
#define Arg SYSTEM_RESERVED_NAME
#define Args SYSTEM_RESERVED_NAME
#define As SYSTEM_RESERVED_NAME
#define B SYSTEM_RESERVED_NAME
#define Bs SYSTEM_RESERVED_NAME
#define C SYSTEM_RESERVED_NAME
#define Cp SYSTEM_RESERVED_NAME
#define Cs SYSTEM_RESERVED_NAME
...

mstorsjo · 2025-10-03T09:04:26Z

I filed #161808 for the new issue that cropped up.

…es (#161817) This mirrors a similar test that libcxx does, to make sure that the libcxx headers don't use any unreserved symbols. The header for polluting with defines is based very far on the libcxx one; some parts of it could possibly be omitted, but I included most of it for completeness here. This should allow catching these issues earlier, to avoid issues like #161808 and #98478 happening again.

[clang] [Headers] Don't use unreserved name in avx512fp16intrin.h

dccf2ca

This can cause breakage with user code that does "#define A ...".

mstorsjo requested review from AaronBallman and phoebewang July 11, 2024 13:17

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Jul 11, 2024

AaronBallman reviewed Jul 11, 2024

View reviewed changes

Fix another case of an unreserved identifier

12c1633

KanRobert reviewed Jul 11, 2024

View reviewed changes

AaronBallman approved these changes Jul 11, 2024

View reviewed changes

mstorsjo merged commit 6f04f46 into llvm:main Jul 11, 2024

mstorsjo deleted the clang-avx512-header-reserved-names branch July 11, 2024 20:45

aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024

[clang] [Headers] Don't use unreserved names in avx512fp16intrin.h (l…

fcb295f

…lvm#98478) This can cause breakage with user code that does "#define A ...".

mstorsjo mentioned this pull request Oct 3, 2025

[Clang] Clang headers use unreserved identifiers #161808

Closed

mstorsjo mentioned this pull request Oct 3, 2025

[clang] [test] Verify that intrinsic headers don't use unreserved names #161817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] [Headers] Don't use unreserved name in avx512fp16intrin.h #98478

[clang] [Headers] Don't use unreserved name in avx512fp16intrin.h #98478

Uh oh!

mstorsjo commented Jul 11, 2024

Uh oh!

llvmbot commented Jul 11, 2024 •

edited

Loading

Uh oh!

KanRobert commented Jul 11, 2024

Uh oh!

mstorsjo commented Jul 11, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

mstorsjo commented Jul 11, 2024

Uh oh!

KanRobert left a comment

Uh oh!

AaronBallman left a comment

Uh oh!

mstorsjo commented Oct 3, 2025

Uh oh!

RKSimon commented Oct 3, 2025

Uh oh!

mstorsjo commented Oct 3, 2025

Uh oh!

mstorsjo commented Oct 3, 2025

Uh oh!

Uh oh!

[clang] [Headers] Don't use unreserved name in avx512fp16intrin.h #98478

[clang] [Headers] Don't use unreserved name in avx512fp16intrin.h #98478

Uh oh!

Conversation

mstorsjo commented Jul 11, 2024

Uh oh!

llvmbot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KanRobert commented Jul 11, 2024

Uh oh!

mstorsjo commented Jul 11, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

mstorsjo commented Jul 11, 2024

Uh oh!

KanRobert left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

mstorsjo commented Oct 3, 2025

Uh oh!

RKSimon commented Oct 3, 2025

Uh oh!

mstorsjo commented Oct 3, 2025

Uh oh!

mstorsjo commented Oct 3, 2025

Uh oh!

Uh oh!

llvmbot commented Jul 11, 2024 •

edited

Loading