[PATCH] D131873: [PowerPC] Optimize compare by using record form in post-RA.

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

This patch misses MIR cases for this transformation. See example in `llvm/test/CodeGen/PowerPC/fold-frame-offset-using-rr.mir`



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2776
+  int64_t CmpMask, CmpValue;
+  if (!analyzeCompare(CmpMI, SrcReg, SrcReg2, CmpMask, CmpValue) ||
+      SrcReg.isVirtual() || SrcReg2.isVirtual() || SrcReg2 || CmpValue)
----------------
Better to add comments to explicitly specify the condition for optimizing, eg:
1: this should be a compare instruction between a reg and an imm
2: the imm should be 0


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2777
+  if (!analyzeCompare(CmpMI, SrcReg, SrcReg2, CmpMask, CmpValue) ||
+      SrcReg.isVirtual() || SrcReg2.isVirtual() || SrcReg2 || CmpValue)
+    return false;
----------------
nit: `isVirtual()` is not needed since this is for post RA.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2780
+
+  // CmpMI can't be deleted if it has implicit def.
+  if (CmpMI.hasImplicitDef())
----------------
Add a MIR case for this?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2786
+  MachineInstr *SrcMI = getDefMIPostRA(SrcReg, CmpMI, OtherIntermediateUse);
+  if (OtherIntermediateUse || !SrcMI || SrcMI->getParent() != CmpMI.getParent())
+    return false;
----------------
`getDefMIPostRA()` would not return a def in another block, so is `SrcMI->getParent() != CmpMI.getParent()` needed?

```
SrcReg = op1
xx1 = op2 SrcReg
xx2= cmp SrcReg
```

Intermediate use of `SrcReg` should not stop the compare optimization?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2799
+      SrcMI->definesRegister(CRReg))
+    return false;
+
----------------
If there is any use of cr0 between SrcMI and CmpMI, we should also bail out? We will define the cr0 in the new record form or SrcMI?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2803
+  int NewOpC = PPC::getRecordFormOpcode(OldOpc);
+  if (NewOpC == -1 || OldOpc == NewOpC)
+    return false;
----------------
I can not image a case where `OldOpc == NewOpC`, could you please give an example here?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2806
+
+  for (auto &CompareUseMI : MRI->use_instructions(CRReg)) {
+    unsigned UseOpc = CompareUseMI.getOpcode();
----------------
I don't understand the logic here, and what result will `MRI->use_instructions()` produce after RA?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2821
+  // Fix up killed/dead flag after transformation.
+  if (IsFwdFeederRegKilled || RegMO.isKill())
+    fixupIsDeadOrKill(SrcMI, &CmpMI, CRReg);
----------------
`RegMO` is the def of CmpMI, it will not have kill flag. And CR0 will be be killed between SrcMI and CmpMI, otherwise we can not do the transformation. No use of CR0 should exist between SrcMI and CmpMI.

Here I think we should handle the killed flag for `SrcReg` if it is killed in CmpMI and there is other use between SrcMI and CmpMI? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131873