Page MenuHomePhabricator

[X86] Add -mgeneral-regs-only support.
ClosedPublic

Authored by tianqing on Jun 8 2021, 11:32 PM.

Diff Detail

Unit TestsFailed

TimeTest
750 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/ompt /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

tianqing created this revision.Jun 8 2021, 11:32 PM
tianqing requested review of this revision.Jun 8 2021, 11:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 11:32 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I don't know much about target feature inheritance - does this guarantee that the entire sse/avx/avx512 level chain is correctly disabled?

tianqing updated this revision to Diff 352616.Jun 16 2021, 8:31 PM

Respect order of options.

I don't know much about target feature inheritance - does this guarantee that the entire sse/avx/avx512 level chain is correctly disabled?

setFeatureEnabled queries ImpliedFeatures to disable all dependent features.

pengfei added inline comments.Jun 21 2021, 12:47 AM
clang/include/clang/Driver/Options.td
3214–3215

Will this change affect AArch64 or other targets expect AArch64 and x86?

clang/lib/Basic/Targets/X86.cpp
120–133

Why do we need to expand it again after we handling in driver?

121

Please follow Lint's comment.

136–158

Shouldn't this be simply skipped under "general-regs-only"?

clang/lib/Driver/ToolChains/Arch/X86.cpp
216–236

Why we need copy the code here? Can it be simply use:

if (Args.getLastArg(options::OPT_mgeneral_regs_only))
  Features.insert(Features.end(), {"-x87", "-mmx", "-sse"});
handleTargetFeaturesGroup(Args, Features, options::OPT_m_x86_Features_Group);
clang/test/CodeGen/attr-target-general-regs-only-x86.c
15

Why we have a -avx512f when enabling avx2?

clang/test/Driver/x86-mgeneral-regs-only.c
27

ditto

tianqing updated this revision to Diff 353536.Jun 21 2021, 7:50 PM

Fix lint comment.

tianqing marked an inline comment as done.Jun 21 2021, 8:06 PM
tianqing added inline comments.
clang/include/clang/Driver/Options.td
3214–3215

No, using this option on other targets gives "argument unused during compilation" warning.

clang/lib/Basic/Targets/X86.cpp
120–133

The driver handles command line options, and this handles "target" attributes.
Just adding "+general-regs-only" in the driver also works. But it's not in OPT_m_x86_Features_Group, and current handleTargetFeaturesGroup will ignore it so we still have to copy its code, not much of a difference.

(Note AArch64 only supports options)

136–158

It's still about order of options. Seeing a "general-regs-only" doesn't mean the function is really GPR only, we have to apply all options in order and check the result.

clang/lib/Driver/ToolChains/Arch/X86.cpp
216–236

To make sure later options override earlier ones. This is how GCC behaves.

This is demonstrated in the "GPR" and "AVX2" check lines of the new tests.

clang/test/CodeGen/attr-target-general-regs-only-x86.c
15

See llvm::X86::updateImpliedFeatures(), enabling a feature will enable all features it depends on, disabling a feature also disables all features depending on it. This is "target feature inheritance" mentioned in Simon's comment.

pengfei accepted this revision.Jun 23 2021, 12:00 AM

LGTM.

This revision is now accepted and ready to land.Jun 23 2021, 12:00 AM

Thanks - that makes sense - LGTM

This revision was landed with ongoing or failed builds.Jun 29 2021, 1:05 AM
This revision was automatically updated to reflect the committed changes.