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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Mostly minor comments and a couple of questions.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:1398
+  bool Changed = onlyFoldImmediate(UseMI, DefMI, Reg);
+  if (Changed && DeleteDef)
     DefMI.eraseFromParent();
----------------
I know that this is how it was done in the past but how about just checking:
```
MRI->use_nodbg_empty(Reg)
```
Or maybe there was a good reason to have done it this way in the past and I can't see it. :)


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:329
+        Simplified |= Folded;
+        if (MRI->use_nodbg_empty(MIDestReg) && Folded) {
+          ++NumLoadImmZeroFoldedAndRemoved;
----------------
Why do you need to check `&& Folded` here? The output register of this `LI` instruction has no non-debug uses so you can probably just schedule it to be deleted anyway. If you do that you also don't need to keep `Folded` around and just do what Nemanja mentioned:
```
Simplified |=
  TII->FoldImmediateWithoutDelete(UseMI, MI, MIDestReg, MRI);
```


================
Comment at: llvm/test/CodeGen/PowerPC/fold-remove-li.ll:5
+; RUN:      -verify-machineinstrs < %s | FileCheck %s -check-prefix=CHECK-LE
+; RUN:  llc -mtriple=powerpc64-unknown-linux-gnu -ppc-asm-full-reg-names \
+; RUN:      -verify-machineinstrs < %s | FileCheck %s -check-prefix=CHECK-BE
----------------
nit:
Add `-mcpu` to these tests. If you don't I think that the default for LE is pwr8 and the default for BE is pwr4 (?). Anyway, I think you will get a more reliable test if you specify the cpu. 


================
Comment at: llvm/test/CodeGen/PowerPC/save-crbp-ppc32svr4.ll:17
-; CHECK: stw 28, -24(29)
-; CHECK: stw 12, -28(29)
 
----------------
Question: 
Why do we lose a store here? Is it because we remove an LI instruction and then that reduces register pressure? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69168