Page MenuHomePhabricator

[RISCV] Refactor and improve eliminateFrameIndex.
ClosedPublic

Authored by craig.topper on Sep 30 2022, 8:00 PM.

Details

Summary

There are few changes mixed in here.

-Try to reuse the destination register from ADDI instead of always
creating a virtual register. This way we lean on the register
scavenger in fewer case.
-Explicitly reuse the primary virtual register when possible. There's
still a case where both getVLENFactoredAmount and handling large
fixed offsets can both create a secondary virtual register.
-Combine similar BuildMI calls by manipulating the Register variables.

There are still a couple early outs for ADDI, but overall I tried to
arrange the code into steps.

Diff Detail

Event Timeline

craig.topper created this revision.Sep 30 2022, 8:00 PM
craig.topper requested review of this revision.Sep 30 2022, 8:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 8:00 PM
reames added inline comments.Oct 3 2022, 8:21 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
187

I don't follow this logic. Why is knowing it's an add enough to know we can reuse/rewrite it? Actually, I see this part is not new in the code, it's just moved up from above. I'd still encourage you to improve the comment. :)

245

Can you land a pre-change which just does the change in code structure here? This diff is very hard to read. No separate review needed.

craig.topper added inline comments.Oct 3 2022, 8:43 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
187

It is new. The old code only reuses the ADDI destination for the last instruction in the sequence. The change is to use it as a scratch register for the whole sequence.

There are only 3 types of instructions that we can have here: ADDI, load, or store.

The only inputs to the ADDI are the frame index, and the immediate and we know it doesn't read it's own destination register. That means the destination register is dead before it is written. Thus we can use it as a scratch register. AArch64 does this from what I could tell in their code. I suppose we could do the same for loads, but not stores.

245

I'll try, but it is kind of related to the how the registers are managed in this patch. So I'm not sure it fits into the original code by itself.

craig.topper planned changes to this revision.Oct 3 2022, 11:43 AM

Re-base after splitting to D135092.

craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
209

I wonder if there's any reason we can't add the scalable offset here instead of adding large fixed offset first.

HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
209

I think there is no strong reason to do so. eliminateFrameIndex will handle all kinds of FrameIndex operand. At the beginning, we only have fixed offset in the FrameIndex. Handle large offset is the existing code. When implementing RVV frame handling, I just put the RVV related code after the existing code at that time. I wonder if there is any benefit to put the scalable part earlier? If so, I think it makes sense to move it before handling large fixed offset.

reames accepted this revision.Oct 4 2022, 7:41 AM

LGTM

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
209

@craig.topper I agree that reordering looks legal to me. Would certainly simplify the code.

This revision is now accepted and ready to land.Oct 4 2022, 7:41 AM
This revision was landed with ongoing or failed builds.Oct 4 2022, 9:35 AM
This revision was automatically updated to reflect the committed changes.