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

[pengfei]

Summary

AMD64 ABI mandates caller to specify the number of used SSE registers
when passing variable arguments.
GCC also provides option -mskip-rax-setup to skip the setup of rax when
SSE is disabled. This helps to reduce the code size, see pr23258.

[joerg]

llvm/lib/Target/X86/X86ISelLowering.cpp

99

The description needs some further clarification. It should likely be "can be passed" otherwise it begs the question of when the conditional statement is false.

[pengfei]

Address review comment.

llvm/lib/Target/X86/X86ISelLowering.cpp

99

Good suggestion. Done, thanks.

[nickdesaulniers]

Fixes https://bugs.llvm.org/show_bug.cgi?id=23258?

[pengfei]

In D112413#3089073, @nickdesaulniers wrote:

Fixes https://bugs.llvm.org/show_bug.cgi?id=23258?

Yes.

[nickdesaulniers]

In D112413#3089073, @nickdesaulniers wrote:

Fixes https://bugs.llvm.org/show_bug.cgi?id=23258?

sorry, I missed the PR # in the description; my mistake.

[nickdesaulniers]

Thanks for the patch; I'd like to see this encoded in the IR (perhaps as a function level attribute), the -mno- variant added, and perhaps help the user if they forget to add -mno-sse that -mskip-rax-setup will otherwise be ignored.

Just noting:

For the Linux kernel, arch/x86/Makefile:51 sets -mno-sse (line 117 then sets -mskip-rax-setup if the compiler supports it).

Looking at the GCC patch and playing with it in godbolt, it looks like -mskip-rax-setup is only respected when -mno-sse is set.

clang/include/clang/Driver/Options.td

3193

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.

clang/lib/Driver/ToolChains/Clang.cpp

2197

It might be nice to warn the user if this flag depends on -mno-sse.

llvm/lib/Target/X86/X86ISelLowering.cpp

96

If it's a command line option rather than encoded in the IR, then this won't be handled correctly under LTO (we support building the x86 linux kernel under LTO), unless the linker re-passes the flag. I've been trying to avoid that by encoding this information properly in IR rather than rely on codegen command line options which I find brittle. Please see https://reviews.llvm.org/D103928 as an example.

[MaskRay]

clang/include/clang/Driver/Options.td

3193

Consider SimpleMFlag

llvm/lib/Target/X86/X86ISelLowering.cpp

96

This depends on how frequent the feature is used.

I see that arch/x86/Makefile may add -mskip-rax-setup which is presumably used by all C files, so a module flag metadata suffices. If there are mix-and-match cases for different modules (e.g. ssp/nossp) a function attribute may be used. If a feature is really obscure than I might not consider wasting an IR attribute on it. Arguably if a feature is sufficient obscure we may not want to port it from GCC at all.

So "This fixes pr23258." in the description should state the use case to give archaeologists a hint why the feature is added.

[nickdesaulniers]

llvm/lib/Target/X86/X86ISelLowering.cpp

96

That's fine, though then we should add a test for new module level IR using llvm-link, IMO, since this (and many -m flags) imply an ABI breakage across modules with mismatched use of this flag.

Arguably if a feature is sufficient obscure we may not want to port it from GCC at all.

In this case, I [e-mail] has some nice numbers to justify this feature in their patch. I can run similar numbers on clang built kernels, but it seems nice to have.

[pengfei]

Address review comments.

1. Add support for -mno-skip-rax-setup and its test;
2. Use module flag metadata instead of command line option in backend;

[pengfei]

clang/include/clang/Driver/Options.td

3193

Thanks for the suggestion. I think adding a -mno-skip-rax-setup is simply here.

clang/lib/Driver/ToolChains/Clang.cpp

2197

GCC doesn't warn it either.

[nickdesaulniers]

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

clang/include/clang/Basic/CodeGenOptions.def

455 ↗

(On Diff #387147)

Maybe note that this is x86 specific?

clang/include/clang/Driver/Options.td

3193

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)?

llvm/test/CodeGen/X86/pr23258.ll

4 ↗

(On Diff #387147)

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.

17 ↗

(On Diff #387147)

might be able to remove entry:, dso_local, and nounwind?

22 ↗

(On Diff #387147)

Is there anything one the callee side that we should check? I assume the callee does some check of %al normally?

[nickdesaulniers]

llvm/test/CodeGen/X86/pr23258.ll

22 ↗

(On Diff #387147)

s/one/on/

[pengfei]

Address review comments.

[pengfei]

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.

clang/include/clang/Driver/Options.td

3193

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.

llvm/test/CodeGen/X86/pr23258.ll

4 ↗

(On Diff #387147)

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.

[nickdesaulniers]

Thanks for the feature. I'd still prefer to see SimpleMFlag used, but it's not a big deal to me.

clang/include/clang/Driver/Options.td

3193

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.

While clang does try hard to emulate GCC, both in features and interface, that's trying too hard to emulate poor interface.

[pengfei]

Thanks @nickdesaulniers for the review! I found another problem with SimpleMFlag, to which MarshallingInfoFlag cannot be assigned separately. I'll land it as is then.

clang/include/clang/Driver/Options.td

def mms_bitfields : Flag<["-"], "mms-bitfields">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard">,
   MarshallingInfoFlag<LangOpts<"MSBitfields">>;
 def moutline : Flag<["-"], "moutline">, Group<f_clang_Group>, Flags<[CC1Option]>,
     HelpText<"Enable function outlining (AArch64 only)">;
 def mno_outline : Flag<["-"], "mno-outline">, Group<f_clang_Group>, Flags<[CC1Option]>,
     HelpText<"Disable function outlining (AArch64 only)">;
 def mno_ms_bitfields : Flag<["-"], "mno-ms-bitfields">, Group<m_Group>,
   HelpText<"Do not set the default structure layout to be compatible with the Microsoft compiler standard">;
+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]>,
   HelpText<"Force realign the stack at entry to every function">,
   MarshallingInfoFlag<CodeGenOpts<"StackRealignment">>;
 def mstack_alignment : Joined<["-"], "mstack-alignment=">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set the stack alignment">,
   MarshallingInfoInt<CodeGenOpts<"StackAlignment">>;
 def mstack_probe_size : Joined<["-"], "mstack-probe-size=">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Set the stack probe size">,

clang/lib/Driver/ToolChains/Clang.cpp

if (Value == "intel" || Value == "att") {
       D.Diag(diag::err_drv_unsupported_option_argument)
           << A->getOption().getName() << Value;
     }
   } else if (D.IsCLMode()) {
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-x86-asm-syntax=intel");
   }
 
+  if (Args.hasArg(options::OPT_mskip_rax_setup)) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back("-x86-skip-rax-setup");
+  }
+
   // Set flags to support MCU ABI.
   if (Args.hasFlag(options::OPT_miamcu, options::OPT_mno_iamcu, false)) {
     CmdArgs.push_back("-mfloat-abi");
     CmdArgs.push_back("soft");
     CmdArgs.push_back("-mstack-alignment=4");
   }
 
   // Handle -mtune.

clang/test/Driver/x86_features.c

 // RUN: %clang -target i386-unknown-unknown -### -S %s -msse -msse4 -mno-sse -mno-mmx -msse  2>&1 | FileCheck %s
 // CHECK: "pentium4" "-target-feature" "+sse4.2" "-target-feature" "-mmx" "-target-feature" "+sse"
 // Note that we filter out all but the last -m(no)sse.
 
 // Test that we don't produce an error with -mieee-fp.
 // RUN: %clang -### %s -mieee-fp -S 2>&1 | FileCheck --check-prefix=IEEE %s
 // IEEE-NOT: error: unknown argument
+
+// RUN: %clang -### %s -mskip-rax-setup -S 2>&1 | FileCheck --check-prefix=SRS %s
+// SRS: "-mllvm" "-x86-skip-rax-setup"

llvm/lib/Target/X86/X86ISelLowering.cpp

 
 static cl::opt<bool> ExperimentalUnorderedISEL(
     "x86-experimental-unordered-atomic-isel", cl::init(false),
     cl::desc("Use LoadSDNode and StoreSDNode instead of "
              "AtomicSDNode for unordered atomic loads and "
              "stores respectively."),
     cl::Hidden);
 
+static cl::opt<bool> SkipRaxSetup(
+    "x86-skip-rax-setup", cl::init(false),
+    cl::desc("Skips setting up the RAX register when SSE is disabled. This "
+             "option has nothing to do with 32 bits or SSE enabled case."),
+    cl::Hidden);
+
 /// Call this when the user attempts to do something unsupported, like
 /// returning a double without SSE2 enabled on x86_64. This is not fatal, unlike
 /// report_fatal_error, so calling code should attempt to recover without
 /// crashing.
 static void errorUnsupported(SelectionDAG &DAG, const SDLoc &dl,
                              const char *Msg) {
   MachineFunction &MF = DAG.getMachineFunction();
   DAG.getContext()->diagnose(
if (!isTailCall) {
       if (G && !G->getGlobal()->hasLocalLinkage() &&
           G->getGlobal()->hasDefaultVisibility())
         Callee = LowerGlobalAddress(Callee, DAG);
       else if (isa<ExternalSymbolSDNode>(Callee))
         Callee = LowerExternalSymbol(Callee, DAG);
     }
   }
 
-  if (Is64Bit && isVarArg && !IsWin64 && !IsMustTail) {
+  if (Is64Bit && isVarArg && !IsWin64 && !IsMustTail &&
+      (Subtarget.hasSSE1() || !SkipRaxSetup)) {
     // From AMD64 ABI document:
     // For calls that may call functions that use varargs or stdargs
     // (prototype-less calls or calls to functions containing ellipsis (...) in
     // the declaration) %al is used as hidden argument to specify the number
     // of SSE registers used. The contents of %al do not need to match exactly
     // the number of registers, but must be an ubound on the number of SSE
     // registers used and is in the range 0 - 8 inclusive.
 

llvm/lib/Target/X86/pr23258.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; 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 -x86-skip-rax-setup | FileCheck %s --check-prefix=HAS-RAX
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+sse -x86-skip-rax-setup | FileCheck %s --check-prefix=HAS-RAX
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=-sse -x86-skip-rax-setup | FileCheck %s --check-prefix=NO-RAX
+
+define dso_local void @foo() nounwind {
+; HAS-RAX-LABEL: foo:
+; HAS-RAX:       # %bb.0: # %entry
+; HAS-RAX-NEXT:    movl $1, %edi
+; HAS-RAX-NEXT:    xorl %eax, %eax
+; HAS-RAX-NEXT:    jmp bar # TAILCALL
+;
+; NO-RAX-LABEL: foo:
+; NO-RAX:       # %bb.0: # %entry
+; NO-RAX-NEXT:    movl $1, %edi
+; NO-RAX-NEXT:    jmp bar # TAILCALL
+entry:
+  tail call void (i32, ...) @bar(i32 1)
+  ret void
+}
+
+declare dso_local void @bar(i32, ...)