aaron.ballman added a comment.
Basically LG though I had a question about the test case.
================
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:
> > 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.
Yeah, that does seem wrong. Agreed it's not in scope for this patch, but if you
would file an issue in GitHub so we don't lose track of it, that'd be
appreciated.
================
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:
> > 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.
Ahhh, that's a good point about being combined OS + target. Thanks for pointing
that out!
================
Comment at: clang/test/Preprocessor/predefined-macros-no-warnings.c:5
+// warnings suppressed by default.
+// RUN: %clang_cc1 %s -E -o /dev/null -Wsystem-headers -Werror -triple arc
+// RUN: %clang_cc1 %s -E -o /dev/null -Wsystem-headers -Werror -triple xcore
----------------
So the expectation is that any warnings that are emitted would be upgraded to
an error and the test would be flagged as a failure because %clang_cc1 would
return nonzero in that case?
(I was thrown for a loop by not using `-verify` and `//
expected-no-diagnostics`)
Pretty sure `-Eonly` is equivalent (it runs the preprocessor without emitting
output, so no extra overhead from piping to /dev/null).
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
