[PATCH] D40807: [RISCV] Support stack frames and offsets up to 32-bits

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

There are a couple of NFC parts that need extracted here before the semantic change can be easily reviewed.



================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:60
 
+void RISCVFrameLowering::adjustReg(MachineBasicBlock &MBB,
+                                   MachineBasicBlock::iterator MBBI,
----------------
Looks like this is an RFC extraction.  Feel free to separate and land.


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:71
+
+  if (isInt<12>(Val)) {
+    BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), DestReg)
----------------
You could add a case for when Val == 0 and use a reg move.

Hm, reviewing the ISA doc, it doesn't look like there's a register register move?  Ok, add is fine.  :)


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:84
+
+    unsigned ScratchReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
+    uint64_t Hi20 = ((Val + 0x800) >> 12) & 0xfffff;
----------------
This really looks like a helper function along the lines of materializedInt32(Reg, Val).  I'm pretty sure you already have other copies of this code as well.


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:91
+    BuildMI(MBB, MBBI, DL, TII->get(RISCV::ADDI), ScratchReg)
+        .addReg(ScratchReg, RegState::Kill)
+        .addImm(Lo12)
----------------
Is it correct to indicate a kill on a use when we're updating the same scratch reg?


================
Comment at: lib/Target/RISCV/RISCVRegisterInfo.cpp:85
+
+  if (!isInt<12>(Offset)) {
+    // The offset won't fit in an immediate, so use a scratch register instead
----------------
Two problems:
1) You're missing an assert that this is an int32.
2) This is the other use of materializeInt32 I mentioned above.  :)


https://reviews.llvm.org/D40807