This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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

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
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
arsenm added inline comments.Dec 14 2022, 12:23 PM
llvm/lib/Target/X86/X86InstrInfo.cpp
5074–5075

Are these not reserved?

void added inline comments.
llvm/lib/Target/X86/X86InstrInfo.cpp
5074–5075

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?

pengfei added inline comments.Dec 14 2022, 9:41 PM
llvm/lib/Target/X86/X86InstrInfo.td
1384

Seems not needed.

1397

ditto.

craig.topper added inline comments.Dec 14 2022, 9:42 PM
llvm/lib/Target/X86/X86InstrInfo.td
1384

I think it's need to make expandPostRAPseudo get called.

pengfei added inline comments.Dec 14 2022, 9:51 PM
llvm/lib/Target/X86/X86InstrInfo.td
1384

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
1384

Agreed. usesCustomInserter can be removed

pengfei added inline comments.Dec 14 2022, 9:53 PM
llvm/lib/Target/X86/X86InstrInfo.cpp
5074–5075

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
5074–5075

Sorry, I misunderstood the question. Please ignore.

void added a comment.EditedDec 15 2022, 10:13 AM

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

llvm/lib/Target/X86/X86InstrInfo.td
1384

Ah! Okay.

void updated this revision to Diff 483260.Dec 15 2022, 11:27 AM

Update TD file and testcase. Also correct the MIBs to have the correct parameters.

pengfei accepted this revision.Jan 26 2023, 2:42 AM

LGTM.

void updated this revision to Diff 493427.Jan 30 2023, 2:38 PM

Rebase.

This revision was landed with ongoing or failed builds.Jan 30 2023, 3:32 PM
This revision was automatically updated to reflect the committed changes.