[X86] Move RDFLAGS/WRFLAGS expansion until after RA
ClosedPublic
Actions

Authored by void on Dec 14 2022, 12:14 PM.

Details

Reviewers

craig.topper
nickdesaulniers
arsenm
efriedma
pengfei

Commits

rG7d626e7cbb3a: [X86] Move RDFLAGS/WRFLAGS expansion until after RA

Summary

The register allocator may introduce reloads in the middle of reading
and writing the EFLAGS register, due to the RDFLAGS & WRFLAGS pseudos
being expanded before RA. This may cause an issue where the stack
pointer was adjusted but the stack offset for the reload wasn't
accounted for (see [1]).

To avoid this, expand these pseudos after register allocation.

[1] https://github.com/llvm/llvm-project/issues/59102

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	40,150 ms	x64 debian > libFuzzer.libFuzzer::fork.test Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/SimpleTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fork.test.tmp-SimpleTest
	60,050 ms	x64 debian > libFuzzer.libFuzzer::fuzzer-leak.test Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
	60,040 ms	x64 debian > libFuzzer.libFuzzer::minimize_crash.test Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
	60,050 ms	x64 debian > libFuzzer.libFuzzer::value-profile-load.test Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

Event Timeline

void created this revision.Dec 14 2022, 12:14 PM

Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 12:14 PM

Herald added subscribers: StephenFan, pengfei, hiraditya. · View Herald Transcript

void requested review of this revision.Dec 14 2022, 12:14 PM

Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 12:14 PM

Herald added subscribers: llvm-commits, wdng. · View Herald Transcript

arsenm added inline comments.Dec 14 2022, 12:23 PM

llvm/lib/Target/X86/X86InstrInfo.cpp
5075–5076	Are these not reserved?

void added a reviewer: pengfei.Dec 14 2022, 12:24 PM

void added inline comments.

llvm/lib/Target/X86/X86InstrInfo.cpp
5075–5076	Dunno. I'm going off of what the original code was doing...

nickdesaulniers accepted this revision.Dec 14 2022, 1:16 PM

nickdesaulniers added inline comments.

llvm/test/CodeGen/X86/eflags.ll
4–22 ↗	(On Diff #482958)	These are already covered by llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll. Do they add anything here?

This revision is now accepted and ready to land.Dec 14 2022, 1:16 PM

You should be able to teach frame lowering to compute the correct offsets by fixing X86InstrInfo::getSPAdjust(). Maybe that's better than trying to avoid the use of frame indexes between the push and pop?

Harbormaster completed remote builds in B203192: Diff 482958.Dec 14 2022, 2:25 PM

pengfei added inline comments.Dec 14 2022, 9:41 PM

llvm/lib/Target/X86/X86InstrInfo.td
1389	Seems not needed.
1402	ditto.

craig.topper added inline comments.Dec 14 2022, 9:42 PM

llvm/lib/Target/X86/X86InstrInfo.td
1389	I think it's need to make expandPostRAPseudo get called.

pengfei added inline comments.Dec 14 2022, 9:49 PM

llvm/lib/Target/X86/X86InstrInfo.td
1389	It just checks `isPseudo` https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/ExpandPostRAPseudos.cpp#L199-L203

pengfei added inline comments.Dec 14 2022, 9:51 PM

llvm/lib/Target/X86/X86InstrInfo.td
1389	I mean `usesCustomInserter = 1` is not needed since the code is removed from EmitInstrWithCustomInserter.

craig.topper added inline comments.Dec 14 2022, 9:52 PM

llvm/lib/Target/X86/X86InstrInfo.td
1389	Agreed. usesCustomInserter can be removed

pengfei added inline comments.Dec 14 2022, 9:53 PM

llvm/lib/Target/X86/X86InstrInfo.cpp
5075–5076	I think this might be a problem for PostRA.

pengfei added inline comments.Dec 14 2022, 10:04 PM

llvm/lib/Target/X86/X86InstrInfo.cpp
5075–5076	Sorry, I misunderstood the question. Please ignore.

In D140045#3996240, @efriedma wrote:

You should be able to teach frame lowering to compute the correct offsets by fixing X86InstrInfo::getSPAdjust(). Maybe that's better than trying to avoid the use of frame indexes between the push and pop?

That's the correct general fix for this issue. Though I still maintain that this is a good addition. Having reloads occurring in the middle of these two instructions isn't really beneficial.

To be clear, I'll submit a subsequent patch that corrects the offsets. :-)