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

[Zhang Kang via Phabricator via llvm-commits]

ZhangKang marked 2 inline comments as done.
ZhangKang 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:
----------------
shchenz wrote:
> 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.
I have tried to find a new case which don't create by the ppc-early-ret pass, but failed. 

The case uses `BuildMI` to create the instruction BCLR/BCLRn instruction is what we expected. I have checked that only the `ppc-early-ret` pass can create those test case. So I can't find the case without hitting this bug.

Below is the search result for `grep -r "BCLR"`.
```
PPCInstrInfo.cpp:      MI.setDesc(get(PPC::BCLR));
PPCInstrInfo.cpp:      MI.setDesc(get(PPC::BCLRn));
PPCEarlyReturn.cpp:STATISTIC(NumBCLR, "Number of early conditional returns");
PPCEarlyReturn.cpp:              ++NumBCLR;
PPCEarlyReturn.cpp:                  TII->get(J->getOpcode() == PPC::BC ? PPC::BCLR : PPC::BCLRn))
PPCEarlyReturn.cpp:              ++NumBCLR;
PPCReduceCRLogicals.cpp:                : OrigBROpcode == PPC::BCLR ? PPC::BCLRn : PPC::BCLR;
PPCReduceCRLogicals.cpp:  if (BROp == PPC::BC || BROp == PPC::BCLR) {
PPCReduceCRLogicals.cpp:  } else if (BROp == PPC::BCn || BROp == PPC::BCLRn) {
PPCReduceCRLogicals.cpp:    if (Opc == PPC::BC || Opc == PPC::BCn || Opc == PPC::BCLR ||
PPCReduceCRLogicals.cpp:        Opc == PPC::BCLRn)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76042