aaron.ballman 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"); } ---------------- john.brawn wrote: > 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. Ah, okay! I think it's worth adding a test case for that scenario to show we've made a bugfix here, not just an NFC change. ================ Comment at: clang/lib/Basic/Targets/VE.cpp:30-32 - Builder.defineMacro("unix", "1"); - Builder.defineMacro("__unix__", "1"); - Builder.defineMacro("__linux__", "1"); ---------------- john.brawn wrote: > 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);`. Hmmm, we still have calls to this outside of `getOSDefines()` though? https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L1400 https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/Le64.cpp#L28 https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/X86.h#L634 https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/X86.h#L907 should those also be changing (perhaps in a follow-up)? ================ Comment at: clang/lib/Basic/Targets/VE.cpp:35 Builder.defineMacro("__ve__", "1"); - Builder.defineMacro("__STDC_HOSTED__", "1"); - Builder.defineMacro("__STDC__", "1"); ---------------- aaron.ballman wrote: > This is technically a breaking change but I think it's a good breaking > change. The VE target seems to imply that there's never a freestanding mode > unlike the general utility: > https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Frontend/InitPreprocessor.cpp#L431 This might also be worth an explicit test case so it's clear that the behavioral change is intentional. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150966/new/ https://reviews.llvm.org/D150966 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits