[RISCV] Refactor and improve eliminateFrameIndex.

[craig.topper]

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.

[reames]

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]

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]

Re-base after splitting to D135092.

[craig.topper]

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]

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]

LGTM

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

209

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