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

Fri Mar 20 23:15:56 PDT 2020

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:
----------------
steven.zhang wrote:
> ZhangKang wrote:
> > steven.zhang wrote:
> > > ZhangKang wrote:
> > > > 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)
> > > > ```
> > > So，the only way to generate BCLRn is from the transform of BCLR，which has the flag set？If it is that case，it is by intention that not set the flag in td as it is propogate from BCLR。
> > There are 2 methods to create the BCLRn.
> > 1. PPCInstrInfo.cpp
> > ```
> > 1461       MI.setDesc(get(PPC::BCLRn));
> > 1462       MachineInstrBuilder(*MI.getParent()->getParent(), MI).add(Pred[1]);
> > ```
> > This method don't use `BuildMI` to create the new MI, it only modify the `setDesc` to `BCLRn` to create the BCLRn instruction, so it will not hit my patch.
> > 
> > 2. PPCEarlyReturn.cpp
> > ```
> > 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);
> > ```
> > This method uses `BuildMI` to create the new MI, so it will hit my patch. This is the test case I gave.
> > 
> > Once we use `BuildMI` to create the `BCLRn` instruction, it will ht the bug lack of `Uses = [LR, RM]`.
> So, it is indeed generated from BCLR. So, it won't have problems now. From the aspect of TD, we'd better set that flags to make it complete, but please correct the dup of implicit operands first. 
Has updated the patch to remove the redundant implicit operands together.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76042