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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers 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



================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:455
 
+/// Whether to skip RAX setup when passing variable arguments.
+CODEGENOPT(SkipRaxSetup, 1, 0)
----------------
Maybe note that this is x86 specific?


================
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]>,
----------------
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)?


================
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
+
----------------
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.


================
Comment at: llvm/test/CodeGen/X86/pr23258.ll:17
+; NO-RAX-NEXT:    jmp bar # TAILCALL
+entry:
+  tail call void (i32, ...) @bar(i32 1)
----------------
might be able to remove `entry:`, `dso_local`, and `nounwind`?


================
Comment at: llvm/test/CodeGen/X86/pr23258.ll:22
+
+declare dso_local void @bar(i32, ...)
+
----------------
Is there anything one the callee side that we should check? I assume the callee does some check of `%al` normally?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112413/new/

https://reviews.llvm.org/D112413