[PowerPC] Remove the redundant implicit operands in ppc-early-ret pass

[ZhangKang]

Summary

In the ppc-early-ret pass, we have use BuildMI and copyImplicitOps when the branch instructions
can do the early return. But the two functions will add implicit operands twice, this is not correct.

This patch is to remove the redundant the implicit operands in ppc-early-ret pass.

[shchenz]

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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?

[ZhangKang]

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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.

[shchenz]

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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.

[ZhangKang]

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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)

[steven.zhang]

llvm/lib/Target/PowerPC/PPCInstrInfo.td

1521 ↗

(On Diff #249822)

Please explain more about why we use the rm ？

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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。

[ZhangKang]

llvm/lib/Target/PowerPC/PPCInstrInfo.td

1521 ↗

(On Diff #249822)

Now, we have used Uses = [RM] for all return & call instructions in td files.

In the file PPCRegisterInfo.td:

244 // FP rounding mode:  bits 30 and 31 of the FP status and control register
245 // This is not allocated as a normal register; it appears only in
246 // Uses and Defs.  The ABI says it needs to be preserved by a function,
247 // but this is not achieved by saving and restoring it as with
248 // most registers, it has to be done in code; to make this work all the
249 // return and call instructions are described as Uses of RM, so instructions
250 // that do nothing but change RM will not get deleted.
251 def RM: PPCReg<"**ROUNDING MODE**">;
252

From the ELF V2 ABI 2.2.1.2,

The bits composing these bit fields are identified as limited access because this ABI manages how they are to be modified and preserved across function calls. Limited-access bits may be changed across function calls only if the called function has specific permission to do so as indicated by the following conditions. A function without permission to change the limited-access bits across a function call shall save the value of the register before modifying the bits and restore it before returning to its calling function.

Standard library functions expressly defined to change the state of limited-access bits are not constrained by nonvolatile preservation rules; for example, the fesetround( ) and feenableexcept( ) functions.

And ABI have not said branch & call instructions will use [RM]. Here, we set all return & call instructions use [RM], this method is a hack to let the Rounding Mode modified be preserved across function calls, it's conservative,

If we can handle Standard library functions expressly defined to change the state of limited-access bits are not constrained by nonvolatile preservation rules well, we can remove the Uses = [RM] for return & call instructions.

The Standard library functions to modify the rounding mode is std::fesetround.

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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].

[steven.zhang]

llvm/lib/Target/PowerPC/PPCInstrInfo.td

1521 ↗

(On Diff #249822)

Thank you for the detail information. Make sense.

llvm/test/CodeGen/PowerPC/early-ret.mir

68

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.

[ZhangKang]

Update the patch to remove the redundant implicit operands in ppc-early-ret pass.

[ZhangKang]

llvm/test/CodeGen/PowerPC/early-ret.mir

68

Has updated the patch to remove the redundant implicit operands together.

[ZhangKang]

Fix the format.

[ZhangKang]

Rebase and add a new test case.

[steven.zhang]

llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp

95

I still have concern on expand the BuildMI here. If it is the problem of copyImplicitOps, we need to add some new API or extend it to support the merge. And I don't think your new solution fix this issue as where is the implicit operands of BCCLR ?

[ZhangKang]

llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp

95

The new patch has used setDesc to avoid using the copyImplicitOps.

[ZhangKang]

Rebase.

[jsji]

LGTM. But please update the summary and description to reflect the actual patch -- it is no longer about adding the RM, but removing the redundant implicit operands in ppc-early-ret pass.

llvm/lib/Target/PowerPC/PPCEarlyReturn.cpp

bool processBlock(MachineBasicBlock &ReturnMBB) {
         for (MachineBasicBlock::iterator J = (*PI)->getLastNonDebugInstr();;) {
           if (J == (*PI)->end())
             break;
 
           if (J->getOpcode() == PPC::B) {
             if (J->getOperand(0).getMBB() == &ReturnMBB) {
               // This is an unconditional branch to the return. Replace the
               // branch with a blr.
-              BuildMI(**PI, J, J->getDebugLoc(), TII->get(I->getOpcode()))
-                  .copyImplicitOps(*I);
+              MachineInstr *MI = ReturnMBB.getParent()->CloneMachineInstr(&*I);
+              (*PI)->insert(J, MI);
+
               MachineBasicBlock::iterator K = J--;
               K->eraseFromParent();
               BlockChanged = true;
               ++NumBLR;
               continue;
             }
           } else if (J->getOpcode() == PPC::BCC) {
             if (J->getOperand(2).getMBB() == &ReturnMBB) {
               // This is a conditional branch to the return. Replace the branch
               // with a bclr.
-              BuildMI(**PI, J, J->getDebugLoc(), TII->get(PPC::BCCLR))
+              MachineInstr *MI = ReturnMBB.getParent()->CloneMachineInstr(&*I);
+              MI->setDesc(TII->get(PPC::BCCLR));
+              MachineInstrBuilder(*ReturnMBB.getParent(), MI)
                   .add(J->getOperand(0))
-                  .add(J->getOperand(1))
-                  .copyImplicitOps(*I);
+                  .add(J->getOperand(1));
+              (*PI)->insert(J, MI);
+
               MachineBasicBlock::iterator K = J--;
               K->eraseFromParent();
               BlockChanged = true;
               ++NumBCLR;
               continue;
             }
           } else if (J->getOpcode() == PPC::BC || J->getOpcode() == PPC::BCn) {
             if (J->getOperand(1).getMBB() == &ReturnMBB) {
               // This is a conditional branch to the return. Replace the branch
               // with a bclr.
-              BuildMI(
-                  **PI, J, J->getDebugLoc(),
-                  TII->get(J->getOpcode() == PPC::BC ? PPC::BCLR : PPC::BCLRn))
-                  .add(J->getOperand(0))
-                  .copyImplicitOps(*I);
+              MachineInstr *MI = ReturnMBB.getParent()->CloneMachineInstr(&*I);
+              MI->setDesc(
+                  TII->get(J->getOpcode() == PPC::BC ? PPC::BCLR : PPC::BCLRn));
+              MachineInstrBuilder(*ReturnMBB.getParent(), MI)
+                  .add(J->getOperand(0));
+              (*PI)->insert(J, MI);
+
               MachineBasicBlock::iterator K = J--;
               K->eraseFromParent();
               BlockChanged = true;
               ++NumBCLR;
               continue;
             }
           } else if (J->isBranch()) {
             if (J->isIndirectBranch()) {

llvm/test/CodeGen/PowerPC/early-ret-verify.mir

bb.3:
     renamable $cr5lt = CRAND killed renamable $cr0eq, killed renamable $cr1lt, implicit $cr1, implicit $cr0
     BCn killed renamable $cr5lt, %bb.2
   
   bb.4:
     BLR implicit $lr, implicit $rm
 
   ; CHECK-LABEL: testEarlyRet
   ; CHECK: bb.0.entry:
-  ; CHECK:   BCLR undef renamable $cr5lt, implicit $lr, implicit $rm, implicit $lr, implicit $rm
+  ; CHECK:   BCLR undef renamable $cr5lt, implicit $lr, implicit $rm
   ; CHECK: bb.1:
   ; CHECK:   renamable $r3 = IMPLICIT_DEF
   ; CHECK:   renamable $r4 = IMPLICIT_DEF
   ; CHECK: bb.2:
   ; CHECK:   $r5 = OR $r4, $r4
   ; CHECK:   renamable $r4 = ADDIC killed $r4, 1, implicit-def $carry
   ; CHECK:   $r6 = OR $r3, $r3
   ; CHECK:   renamable $r3 = ADDZE killed $r3, implicit-def dead $carry, implicit killed $carry

llvm/test/CodeGen/PowerPC/early-ret.mir

bb.2.entry:
     liveins: $r3, $r4, $cr0eq, $cr0lt, $cr0un, $cr0, $cr0gt, $r3, $r3, $r4, $r3, $cr0gt
 
     BLR implicit $lr, implicit $rm, implicit killed $r3
 
   ; CHECK-LABEL: name: testBLR
   ; CHECK: bb.0.entry:
   ; CHECK:   renamable $cr0 = CMPWI renamable $r3, 0
   ; CHECK:   BC killed renamable $cr0gt, %bb.1
-  ; CHECK:   BLR implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $r3
+  ; CHECK:   BLR implicit $lr, implicit $rm, implicit killed $r3
   ; CHECK: bb.1.entry:
   ; CHECK:   renamable $r3 = ADDI killed renamable $r4, 0
   ; CHECK:   BLR implicit $lr, implicit $rm, implicit killed $r3
 ...
 
 
 ---
 name:            testBCLRn
body:             |
   bb.3:
     liveins: $v2
 
     BLR implicit $lr, implicit $rm, implicit killed $v2
 
   ; CHECK-LABEL: name: testBCLRn
   ; CHECK: bb.0.entry:
   ; CHECK:   renamable $cr0 = FCMPUS killed renamable $f3, killed renamable $f4
   ; CHECK:   BCLRn killed renamable $cr0eq, implicit $lr, implicit $rm, implicit killed $v2
   ; CHECK: bb.1.entry:
   ; CHECK:   renamable $cr0 = FCMPUS killed renamable $f1, killed renamable $f2
   ; CHECK:   BCLR killed renamable $cr0eq, implicit $lr, implicit $rm, implicit killed $v2
   ; CHECK: bb.2:
   ; CHECK:   $v2 = VOR killed $v3, $v3
   ; CHECK:   BLR implicit $lr, implicit $rm, implicit killed $v2
 ...
 
body:             |
   bb.3:
     liveins: $v2
 
     BLR implicit $lr, implicit $rm, implicit killed $v2
 
   ; CHECK-LABEL: name: testBCLR
   ; CHECK: bb.0.entry:
   ; CHECK:   renamable $cr0 = FCMPUS killed renamable $f3, killed renamable $f4
-  ; CHECK:   BCLR killed renamable $cr0eq, implicit $lr, implicit $rm, implicit $lr, implicit $rm, implicit killed $v2
+  ; CHECK:   BCLR killed renamable $cr0eq, implicit $lr, implicit $rm, implicit killed $v2
   ; CHECK: bb.1.entry:
   ; CHECK:   renamable $cr0 = FCMPUS killed renamable $f1, killed renamable $f2
   ; CHECK:   BCLRn killed renamable $cr0eq, implicit $lr, implicit $rm, implicit killed $v2
   ; CHECK: bb.2:
   ; CHECK:   $v2 = VOR killed $v3, $v3
   ; CHECK:   BLR implicit $lr, implicit $rm, implicit killed $v2
 ...
 
body:             |
 
   bb.2:
     BLR implicit $lr, implicit $rm
 
   ; CHECK-LABEL: name: testBCCLR
   ; CHECK: bb.0.entry:
   ; CHECK:   renamable $r4 = LI 0
   ; CHECK:   renamable $cr0 = CMPLWI killed renamable $r4, 0
-  ; CHECK:   BCCLR 68, renamable $cr0, implicit $lr, implicit $rm, implicit $lr, implicit $rm
+  ; CHECK:   BCCLR 68, renamable $cr0, implicit $lr, implicit $rm
   ; CHECK: bb.1:
-  ; CHECK:   BCCLR 68, killed renamable $cr0, implicit $lr, implicit $rm, implicit $lr, implicit $rm
+  ; CHECK:   BCCLR 68, killed renamable $cr0, implicit $lr, implicit $rm
   ; CHECK:   BLR implicit $lr, implicit $rm
 ...