[PATCH] D145343: [AMDGPU] Emit predefined macro `__AMDGCN_CUMODE`

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl added inline comments.


================
Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:318
   Builder.defineMacro("__AMDGCN_WAVEFRONT_SIZE", Twine(WavefrontSize));
+  Builder.defineMacro("__AMDGCN_CUMODE", Twine(CUMode));
 }
----------------
arsenm wrote:
> Why do we sometimes use __ on both sides, and sometimes only leading __?
We did not establish a naming convention for pre-defined macros.

The rationale of only prefixing with `__` is that:

1. it is shorter

2. many other targets following that for target feature related macros, e.g.  https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/arm-target-features.c , https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/riscv-target-features.c

The rationale of adding `__` on both sides is:

1. most gcc predefined macros follow this convention https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

2. some targets follow this convention for target feature related macros, e.g https://github.com/llvm/llvm-project/blob/main/clang/test/Preprocessor/x86_target_features.c

I think it is not too late to establish a naming convention for predefined macros for amdgpu target. I would prefer to adding `__` on both sides since

1. most of the existing amdgpu predefined macros follow that. We just need to add `__AMDGCN_WAVEFRONT_SIZE__` and phasing out `__AMDGCN_WAVEFRONT_SIZE`. This will be the least interruptive.

2. we will conform to GCC naming convention for pre-defined macros.

Or maybe we should have an RFC for this topic.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145343/new/

https://reviews.llvm.org/D145343