[PATCH] D135009: [RISCV] Refactor and improve eliminateFrameIndex.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:187
+  Register DestReg;
+  if (MI.getOpcode() == RISCV::ADDI)
+    DestReg = MI.getOperand(0).getReg();
----------------
reames wrote:
> 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.  :)
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.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:241
+  // Add in the scalable offset which has already been computed in DestReg.
+  if (Offset.getScalable()) {
+    assert(DestReg && "DestReg should be valid");
----------------
reames wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135009/new/

https://reviews.llvm.org/D135009