pengfei added a comment. > It might be nice to add an llvm-link test that produces an error when this > module metadata doesn't match. Basically, it would be nice to error if during > LTO we link together two modules that differ in their use of -mskip-rax-setup > since that has ABI implications between the two modules. > https://llvm.org/docs/LangRef.html#module-flags-metadata
I don't think so. The flag looks like ABI breaker, but indeed it's just for performance. If some modules generated without the flag, there's only a bit performance lose. If two modules with and without the flag are linking together. The `Override` works correctly for them. ================ Comment at: clang/include/clang/Driver/Options.td:3193 +def mskip_rax_setup : Flag<["-"], "mskip-rax-setup">, Group<m_Group>, Flags<[NoXarchOption]>, + HelpText<"Skip setting up RAX register when passing variable arguments (x86 only)">; def mstackrealign : Flag<["-"], "mstackrealign">, Group<m_Group>, Flags<[CC1Option]>, ---------------- nickdesaulniers wrote: > pengfei wrote: > > MaskRay wrote: > > > nickdesaulniers wrote: > > > > I think GCC support `-mno-skip-rax-setup` as well. Can you please add > > > > that (and tests for it) as well? We don't need to actually handle it, > > > > I think, but we shouldn't warn about the flag being unsupported, for > > > > example. > > > Consider `SimpleMFlag` > > Thanks for the suggestion. I think adding a `-mno-skip-rax-setup` is simply > > here. > Via @MaskRay 's suggestion, can we do something like: > > ``` > def skip_rax_setup : SimpleMFlag<"skip-rax-setup", "Skip", "Do not skip", > "setting up RAX register when passing variable arguments (x86 only)">; > ``` > > Do we test that this flag is warned about being unused for non-x86 targets? > (Is it worth adding such a test to clang)? Another reason I didn't use `SimpleMFlag` is GCC doesn't have help test for `-mno-skip-rax-setup`. I deliberately hide it in this way. > Do we test that this flag is warned about being unused for non-x86 targets? > (Is it worth adding such a test to clang)? I tested it locally. Driver will warn it for other target, Clang doesn't. I think it should be enough. I don't know if there's precedent, but adding an irrelevant option test to other target's test folder looks a bit weird to me. ================ Comment at: llvm/test/CodeGen/X86/pr23258.ll:4 +; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+sse | FileCheck %s --check-prefix=HAS-RAX +; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=-sse | FileCheck %s --check-prefix=NO-RAX + ---------------- nickdesaulniers wrote: > Might be worth testing the fourth case: +sse, SkipRaxSetup == 0. Though I > think that exposes that `X86TargetLowering::X86TargetLowering` is only > checking whether the Metadata exists, not whether it has a particular value. I don't think so. We use it like the way we use `#ifdef xxx` instead of `#if xxx`. The value can be simply ignored, it is just required by the metadata semantic. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112413/new/ https://reviews.llvm.org/D112413 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
