[PATCH] D76042: [PowerPC] Add the Uses of implicit register for the BCLRn instruction
qshanz via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 16 00:49:18 PDT 2020
steven.zhang added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:1521
+ }
}
----------------
ZhangKang wrote:
> steven.zhang wrote:
> > Please explain more about why we use the rm ？
> 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](http://openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240___RefHeading___Toc377640581.html),
> > 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`.
Thank you for the detail information. Make sense.
================
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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76042/new/
https://reviews.llvm.org/D76042
More information about the llvm-commits
mailing list