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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers requested changes to this revision.
nickdesaulniers added a comment.
This revision now requires changes to proceed.

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 <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/Makefile#n51> sets `-mno-sse` (line 117 <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/Makefile#n117> then sets `-mskip-rax-setup` if the compiler supports it).

Looking at the GCC patch <https://patchwork.ozlabs.org/project/gcc/patch/20141218131150.GA32638@intel.com/> and playing with it in godbolt <https://godbolt.org/z/jMhKE6GMz>, it looks like `-mskip-rax-setup` is only respected when `-mno-sse` is set.



================
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]>,
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2197
 
+  if (Args.hasArg(options::OPT_mskip_rax_setup)) {
+    CmdArgs.push_back("-mllvm");
----------------
It might be nice to warn the user if this flag depends on `-mno-sse`.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:96
 
+static cl::opt<bool> SkipRaxSetup(
+    "x86-skip-rax-setup", cl::init(false),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112413