[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
	1,080 ms	x64 debian > LLVM.CodeGen/X86::eflags.ll Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=x86_64-unknown-linux-gnu < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/eflags.ll \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/eflags.ll
	1,880 ms	x64 debian > LLVM.CodeGen/X86::red-zone.ll Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/red-zone.ll -mcpu=generic -mtriple=x86_64-linux \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/red-zone.ll
	1,630 ms	x64 debian > LLVM.CodeGen/X86::win64_frame.ll Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/win64_frame.ll -mtriple=x86_64-pc-win32 \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/win64_frame.ll
	1,650 ms	x64 debian > LLVM.CodeGen/X86::x86-64-flags-intrinsics.ll Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -verify-machineinstrs -mtriple x86_64-linux < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll
	1,640 ms	x64 debian > LLVM.CodeGen/X86::x86-flags-intrinsics.ll Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -verify-machineinstrs < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/x86-flags-intrinsics.ll \| /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/X86/x86-flags-intrinsics.ll

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	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. :-)