[PATCH] D76042: [PowerPC] Add the Uses of implicit register for the BCLRn instruction

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/early-ret.mir:35
   ; CHECK:   renamable $cr0 = FCMPUS killed renamable $f3, killed renamable $f4
-  ; CHECK:   BCLRn $cr0eq, implicit $lr, implicit $rm, implicit killed $v2
+  ; CHECK:   BCLRn $cr0eq, implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $v2
   ; CHECK: bb.1.entry:
----------------
ZhangKang wrote:
> shchenz wrote:
> > The test change is a little confusing for me. Before your code changes, there should be no use of `lr` and `rm`. But as you can see, there is.
> > 
> > After your change, there are two uses of `lr` and `rm`?
> You can see the line 76 of this case:
> ```
> ; CHECK:   BCLR $cr0eq, implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $v2
> ```
> 
> In fact, the line 35 and line 76 have exposed a bug of the pass `ppc-early-ret`.
> 
> In the file `llvm/llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp`
> ```
> 102           } else if (J->getOpcode() == PPC::BC || J->getOpcode() == PPC::BCn) {
> 103             if (J->getOperand(1).getMBB() == &ReturnMBB) {
> 104               // This is a conditional branch to the return. Replace the branch
> 105               // with a bclr.
> 106               BuildMI(
> 107                   **PI, J, J->getDebugLoc(),
> 108                   TII->get(J->getOpcode() == PPC::BC ? PPC::BCLR : PPC::BCLRn))
> 109                   .addReg(J->getOperand(0).getReg())
> 110                   .copyImplicitOps(*I);
> 111               MachineBasicBlock::iterator K = J--;
> 112               K->eraseFromParent();
> 113               BlockChanged = true;
> 114               ++NumBCLR;
> 115               continue;
> 116             }
> 117           } else if (J->isBranch()) {
> 
> This code do the early return for `BC/BCn + BLR =====> BCLR/BCLRn`.
> For our test case, the I is `BLR implicit $lr, implicit $rm, implicit killed $v2`.
> 
> In the line `110                   .copyImplicitOps(*I);`, 
> we copy all implicit operands from BLR instruction to the new BLCRn, namely, we copy `implicit $lr, implicit $rm, implicit killed $v2` to the new BCLRn instruction.
> 
> Note that the `BuildMI()` function will add the implicit operands automatically if the instruction definition need use implicit operands in the td file.
> .
> For this patch, we add `Uses = [LR, RM]` for the `BCLRn` instruction definition. Namely, the `BuildMI()` function will add `implicit $lr, implicit $rm` to the new BCLRn automatically.
> 
> So you will see `BCLRn $cr0eq, implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $v2`. The `implicit $lr, implicit $rm,` is added by `BuildMI`, and the `implicit $rm, implicit $lr, implicit $rm, implicit killed $v2` is `copyImplicitOps` from the BLR instruction. You can see the line 76 of this case to verify this.
> 
> To fix the bug of `ppc-early-ret` pass, There are two methods.
> 1. Don't use `copyImplicitOps` function, just copy the needed implicit form the BLR instruction, not all implicit registers.
> 2. Use the fuction `removeOperand()` to remove the redundant implicit register.
> The bug will be fixed in the other patch, not this one.
> 
OK, if it is a legacy bug, I think It's better to add a new case without hitting this bug and fix this bug in another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76042