john.brawn marked an inline comment as done.
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:
> 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.
Actually I went and double-checked and I'm wrong, `-march=armv8.N+nowhatever`
doesn't cause __ARM_FEATURE_WHATEVER to be undefined. Which seems wrong, but
it's beyond the scope of this patch.
================
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:
> 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)?
Most of these are TargetInfos that combine OS plus target, e.g.
CygwinARMTargetInfo is both Cygwin OS and ARM Target. The only exception is
Le64, and I have no idea if it's correct or not for it to be doing that.
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
