[PATCH] D81723: [PowerPC] fold addi's imm operand to its imm form consumer's displacement

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2241
   if (MRI->isSSA()) {
+    unsigned OpADDI = 0;
+    unsigned OpLI = 0;
----------------
What about the IdxOfADDI and IdxOfLI ?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2245
+    MachineInstr *MILI = nullptr;
+    MachineInstr *MITmp = nullptr;
     for (int i = 1, e = MI.getNumOperands(); i < e; i++) {
----------------
Move the declaration of MITmp to line 2255. And I prefer to use some meaningful name.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2259
+          OpLI = i;
+          MILI = MITmp;
+        } else if (!OpADDI && (MITmp->getOpcode() == PPC::ADDI ||
----------------
Add "break" here and you don't need to check the !OpLI.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2274
+      OpNoForForwarding = OpADDI;
+      DefMI = MIADDI;
+    }
----------------
I think, you don't need two variables to keep LI and ADDI separate. How about doing it like this:
```
foreach operands
   if (is_li || is_addi) {
      Inst =  MITmp = MRI->getVRegDef(TrueReg);
      idx = i;
     if (is_li)
         break;
   }

if (Inst) {
   OpNoForForwarding = idx;
   DefMI = Inst;
}
```
    


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2326
-  if (MRI.isSSA())
-    return;
 
----------------
Do we still need to fixup the kill/dead flags if it SSA ?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3557
+
+  assert((XFormOpcode != PPC::INSTRUCTION_LIST_END) && "MI must be imm form");
+
----------------
MI must have x-form opcode.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3587
+  int64_t Imm = ImmOperandMI.getImm();
+  if (!isImmElgibleForForwarding(*ImmMO, DefMI, III, Imm))
+    return false;
----------------
We will initialize the Imm inside isImmElgibleForForwarding, and you don't need to initialize it with ImmOperandMI


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81723