This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pengfei on Oct 25 2021, 1:07 AM.

Details

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.

Diff Detail

Event Timeline

pengfei created this revision.Oct 25 2021, 1:07 AM
pengfei requested review of this revision.Oct 25 2021, 1:07 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 25 2021, 1:07 AM
joerg added a subscriber: joerg.Oct 25 2021, 5:09 AM
joerg added inline comments.
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 updated this revision to Diff 381997.Oct 25 2021, 7:49 AM

Address review comment.

llvm/lib/Target/X86/X86ISelLowering.cpp
99

Good suggestion. Done, thanks.

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

nickdesaulniers requested changes to this revision.Oct 27 2021, 11:35 AM

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.

This revision now requires changes to proceed.Oct 27 2021, 11:35 AM
MaskRay added inline comments.Oct 27 2021, 2:52 PM
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.

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 think@hjl.tools 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 updated this revision to Diff 387147.Nov 14 2021, 6:59 PM
pengfei marked 3 inline comments as done.

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 added inline comments.Nov 14 2021, 6:59 PM
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.

pengfei edited the summary of this revision. (Show Details)Nov 14 2021, 7:02 PM

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?

llvm/test/CodeGen/X86/pr23258.ll
22 ↗(On Diff #387147)

s/one/on/

pengfei updated this revision to Diff 387874.Nov 17 2021, 1:52 AM
pengfei marked 3 inline comments as done.

Address review comments.

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 accepted this revision.Nov 17 2021, 10:16 AM

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.

This revision is now accepted and ready to land.Nov 17 2021, 10:16 AM

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.

This revision was automatically updated to reflect the committed changes.