[PATCH] D39785: [PowerPC] Remove redundant register copies in late peephole
Stefan Pintilie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 20 14:01:36 PDT 2018
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
More information about the llvm-commits
mailing list