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.ga32...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
