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

[qshanz via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:1521
+    }
   }
 
----------------
Please explain more about why we use the rm ？


================
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:
> > 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。


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76042