Page MenuHomePhabricator

[PowerPC] Add the Uses of implicit register for the BCLRn instruction
Needs ReviewPublic

Authored by ZhangKang on Wed, Mar 11, 8:35 PM.

Details

Reviewers
kbarton
hiraditya
nemanjai
jsji
steven.zhang
hfinkel
Group Reviewers
Restricted Project
Summary

We have Uses = [LR, RM] for BCLR, but we don't have Uses = [LR, RM] for BCLRn,

The instruction BCLRn is a Branch Conditional to LR instruction, it's obvious that it will use the [LR].

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

We use [RM] for all return and call instructions, this method is a hack to preserve Rounding Mode by a function call. So BCLRn should add the use of [RM].

This patch is to add Uses = [LR, RM] for the BCLRn instruction.

Diff Detail

Event Timeline

ZhangKang created this revision.Wed, Mar 11, 8:35 PM
ZhangKang edited the summary of this revision. (Show Details)Thu, Mar 12, 2:43 AM
shchenz added inline comments.Fri, Mar 13, 2:49 AM
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 marked an inline comment as done.Fri, Mar 13, 7:06 AM
ZhangKang added inline comments.
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 added inline comments.Fri, Mar 13, 7:39 PM
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 marked 2 inline comments as done.Sat, Mar 14, 6:02 AM
ZhangKang added inline comments.
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 added inline comments.Sun, Mar 15, 6:07 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.td
1517

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 marked 6 inline comments as done.Sun, Mar 15, 7:46 PM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
1517

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.

  1. 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 added inline comments.Mon, Mar 16, 12:33 AM
llvm/lib/Target/PowerPC/PPCInstrInfo.td
1517

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 marked 2 inline comments as done.Mon, Mar 16, 12:35 AM

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

ZhangKang marked 2 inline comments as done.Fri, Mar 20, 10:55 PM
ZhangKang added inline comments.
llvm/test/CodeGen/PowerPC/early-ret.mir
68

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

ZhangKang marked an inline comment as done.

Fix the format.