[PATCH] D112413: [X86] Add -mskip-rax-setup support to align with GCC

Phoebe Wang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 17 01:53:28 PST 2021


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



More information about the llvm-commits mailing list