[PATCH] D39785: [PowerPC] Remove redundant register copies in late peephole

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: lib/Target/PowerPC/PPCPreEmitPeephole.cpp:90
+               "The same instruction marked redundant multiple times?");
         MI->eraseFromParent();
         NumRemovedInPreEmit++;
----------------
Once the second `mr` is deleted the first `mr` could become a trivially dead instruction. (It is certainly not guaranteed to be dead but it is possible that it is dead.) Is there a pass after this to clean that up? I think this is just pre-emit so I don't think there is anything else after this. 

It may be worth doing a quick test to see if the first `mr` is dead once we have removed the second one.


================
Comment at: lib/Target/PowerPC/PPCPreEmitPeephole.cpp:130
+
+        // Clear the kill flags from the original instruction.
+        MI.getOperand(1).setIsKill(false);
----------------
Maybe a silly question but... 
I don't understand why these flags are being cleared.
In this case `MI` is the first `mr` instruction. So from what I see:
```
mr r30, r3
```
The `r30` is being overwritten but the `r3` is only a use.  Why would these flags even be set to true in the first place for `r3`? 


================
Comment at: test/CodeGen/PowerPC/remove-cyclic-mr.ll:21
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    mr 30, 3
+; CHECK-NEXT:    addis 12, 2, .L.str at toc@ha
----------------
I'm debating if this test is over-specified.
We are testing 
```
mr 30, 3
... 
mr 3, 30
```
Do we need anything above the first `mr` and below the second `mr`? 
I just don't want this test to fail every now and then when we change something in scheduling or register allocation (or any other pass that might make the code a little different.)



Repository:
  rL LLVM

https://reviews.llvm.org/D39785