[PATCH] D150966: [clang] Don't define predefined macros multiple times
John Brawn via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 22 08:56:10 PDT 2023
john.brawn added inline comments.
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:241-242
Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
- Builder.defineMacro("__ARM_FEATURE_ATOMICS", "1");
- Builder.defineMacro("__ARM_FEATURE_CRC32", "1");
}
----------------
aaron.ballman wrote:
> Hmm, is this correct?
>
> `__ARM_FEATURE_ATOMICS` is defined in one other place, but it's conditionally defined: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/AArch64.cpp#L475
>
> and `__ARM_FEATURE_CRC32` is defined in two places, both conditional: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/AArch64.cpp#L422 and https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/ARM.cpp#L747
>
>
AArch64TargetInfo::setArchFeatures sets HasCRC and HasLSE to true for >= 8.1. This does mean that if you do `-march=armv8.1-a+nocrc` then the current behaviour is that __ARM_FEATURE_CRC32 is defined but the behaviour with this patch is that it's not defined, but the new behaviour is correct as we shouldn't be defining it in that case.
================
Comment at: clang/lib/Basic/Targets/VE.cpp:30-32
- Builder.defineMacro("unix", "1");
- Builder.defineMacro("__unix__", "1");
- Builder.defineMacro("__linux__", "1");
----------------
aaron.ballman wrote:
> Shouldn't this be calling `DefineStd(Builder, "unix", Opts);` like all the others?
Only the OS target should be defining OS-related macros, and in AllocateTarget in Targets.cpp VETargetInfo is hardcoded to always use LinuxTargetInfo, which calls `DefineStd(Builder, "unix", Opts);`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150966/new/
https://reviews.llvm.org/D150966
More information about the cfe-commits
mailing list