[PATCH] D69168: [PowerPC] Fold redundant load immediates of zero and delete if possible

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

Please refactor the common logic out and address the comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1395
+// and only performs the fold.
+bool PPCInstrInfo::FoldImmediateWithoutDelete(MachineInstr &UseMI,
+                                              MachineInstr &DefMI, unsigned Reg,
----------------
We shouldn't do this. We now have two nearly identical implementations of this function. One deletes the def the other one does not. That is the only difference as far as I can tell.

Please refactor the common logic into a single function to form something like this:
```
bool PPCInstrInfo::onlyFoldImm(...) { 
  // All the folding logic.
}
bool PPCInstrInfo::FoldImmediate(...) {
  bool Changed = onlyFoldImm(...);
  if (Changed && MRI->use_nodbg_empty(Reg))
    // Delete the def
  return Changed;
}
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:319
+      case PPC::LI8: {
+        // Go through all the users of this load and for the instructions which
+        // recognize 0 as the number and not the register and replace this load
----------------
This run-on sentence is hard to follow. Break it up a bit please. Perhaps:
```
// If we are materializing a zero, look for any use operands for which zero
// means immediate zero. All such operands can be replaced with PPC::ZERO.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:323
+        // needed.
+        if (!MI.getOperand(1).isImm())
+          break;
----------------
amyk wrote:
> Correct me if I am wrong, but isn't this being checked inside `FoldImmediateWithoutDelete`?
> ```
>  if (!DefMI.getOperand(1).isImm())
>     return false;
>   if (DefMI.getOperand(1).getImm() != 0)
>     return false;
> ```
> Do we need to check this in both places?
We do want to keep it here as well because there may be many users. It is pointless to go through all of them if the materialized value is non-zero. But please do combine these into a single condition:
```
if (!MI.getOperand(1).isImm() || MI.getOperand(1).getImm() != 0)
  break;
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:329
+        for (MachineInstr& UseMI : MRI->use_instructions(MIDestReg)) {
+          bool DeleteDef = MRI->hasOneNonDBGUse(MIDestReg);
+          bool Folded =
----------------
Let's not check this at each iteration. We can just check for no remaining uses after the loop.
Then the body of the loop becomes a single line:
```
Simplified |=
  TII->FoldImmediateWithoutDelete(UseMI, MI, MIDestReg, MRI);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:337
+          Simplified |= Folded;
+        }
+        break;
----------------
Here, we can check for no remaining uses of `MIDestReg` and mark `MI` for deletion if so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69168