[PATCH] D58685: [PowerPC] Remove UseVSXReg
Yi-Hong Lyu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 4 11:53:34 PST 2019
Yi-Hong.Lyu marked 6 inline comments as done.
Yi-Hong.Lyu added a comment.
I `export LLVM_VERIFY_MACHINEINSTRS=1` and run the LIT/LNT tests with my patch and it pass either.
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:167
case MachineOperand::MO_Register: {
- unsigned Reg = PPCInstrInfo::getRegNumForOperand(MI->getDesc(),
- MO.getReg(), OpNo);
-
- const char *RegName = PPCInstPrinter::getRegisterName(Reg);
+ const char *RegName = PPCInstPrinter::getRegisterName(MO.getReg());
----------------
jsji wrote:
> jsji wrote:
> > Yi-Hong.Lyu wrote:
> > > jsji wrote:
> > > > Are we sure we don't need mapping any more?
> > > I believe so. I apply the following patch and there is no failure in LIT/LNT tests. It seems `MO.getReg()` is not changed at all after invoking `getRegNumForOperand`. In contrast, if I keep it, for some inline assembly test case (e.g., `CodeGen/Generic/2007-04-08-MultipleFrameIndices.ll`), `Desc.OpInfo` would be null and I need additional logic to handle this case even the invocation is useless.
> > > ```
> > > $ git diff
> > > diff --git a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> > > index f4ef95c..aeb9c23 100644
> > > --- a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> > > +++ b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> > > @@ -165,7 +165,7 @@ void PPCAsmPrinter::printOperand(const MachineInstr *MI, unsigned OpNo,
> > > switch (MO.getType()) {
> > > case MachineOperand::MO_Register: {
> > > unsigned Reg = PPCInstrInfo::getRegNumForOperand(MI->getDesc(),
> > > - MO.getReg(), OpNo);
> > > + MO.getReg(), OpNo, true);
> > >
> > > const char *RegName = PPCInstPrinter::getRegisterName(Reg);
> > >
> > > diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.h b/llvm/lib/Target/PowerPC/PPCInstrInfo.h
> > > index ee16d43..ebd9024 100644
> > > --- a/llvm/lib/Target/PowerPC/PPCInstrInfo.h
> > > +++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.h
> > > @@ -427,13 +427,17 @@ public:
> > > /// to instructions that use both Altivec and VSX numbering (for different
> > > /// operands).
> > > static unsigned getRegNumForOperand(const MCInstrDesc &Desc, unsigned Reg,
> > > - unsigned OpNo) {
> > > + unsigned OpNo, bool equal = false) {
> > > + unsigned oldReg = Reg;
> > > if (Desc.TSFlags & PPCII::UseVSXReg) {
> > > if (isVRRegister(Reg))
> > > Reg = PPC::VSX32 + (Reg - PPC::V0);
> > > else if (isVFRegister(Reg))
> > > Reg = PPC::VSX32 + (Reg - PPC::VF0);
> > > }
> > > + if (equal) {
> > > + assert(oldReg == Reg && "Reg is altered");
> > > + }
> > > return Reg;
> > > }
> > > };
> > > ```
> > > In contrast, if I keep it, for some inline assembly test case (e.g., CodeGen/Generic/2007-04-08-MultipleFrameIndices.ll), Desc.OpInfo would be null and I need additional logic to handle this case even the invocation is useless.
> >
> > Do we know why it works before but fail now?
> >
> > I apply the following patch and there is no failure in LIT/LNT tests.
>
>
> Interesting. To double confirm, you applied the patch against ToT (without your proposed change of td files in this review), right? I am curious why we add this logic before?
>
> No failure in LIT/LNT tests does NOT prove that this is safe to me.
> Can we get some testcase that are supposed to run into this logic (eg: some MI with UseVSXReg flags set) and check the 'Reg` values returned here? And compare to the `Reg` values with your proposed change?
>
For //CodeGen/Generic/2007-04-08-MultipleFrameIndices.ll// , `%A = call i32 asm sideeffect "inline asm $0 $2 $3 $4", "=r,0,i,m,m"( i32 0, i32 1, i8** %foo, i32** %bar )`, `MI->getOpcode()` is `INLINEASM` and `Desc.OpInfo` would be null. In original version, we don't use `Desc.OpInfo` at all.
================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:167
case MachineOperand::MO_Register: {
- unsigned Reg = PPCInstrInfo::getRegNumForOperand(MI->getDesc(),
- MO.getReg(), OpNo);
-
- const char *RegName = PPCInstPrinter::getRegisterName(Reg);
+ const char *RegName = PPCInstPrinter::getRegisterName(MO.getReg());
----------------
Yi-Hong.Lyu wrote:
> jsji wrote:
> > jsji wrote:
> > > Yi-Hong.Lyu wrote:
> > > > jsji wrote:
> > > > > Are we sure we don't need mapping any more?
> > > > I believe so. I apply the following patch and there is no failure in LIT/LNT tests. It seems `MO.getReg()` is not changed at all after invoking `getRegNumForOperand`. In contrast, if I keep it, for some inline assembly test case (e.g., `CodeGen/Generic/2007-04-08-MultipleFrameIndices.ll`), `Desc.OpInfo` would be null and I need additional logic to handle this case even the invocation is useless.
> > > > ```
> > > > $ git diff
> > > > diff --git a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> > > > index f4ef95c..aeb9c23 100644
> > > > --- a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> > > > +++ b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
> > > > @@ -165,7 +165,7 @@ void PPCAsmPrinter::printOperand(const MachineInstr *MI, unsigned OpNo,
> > > > switch (MO.getType()) {
> > > > case MachineOperand::MO_Register: {
> > > > unsigned Reg = PPCInstrInfo::getRegNumForOperand(MI->getDesc(),
> > > > - MO.getReg(), OpNo);
> > > > + MO.getReg(), OpNo, true);
> > > >
> > > > const char *RegName = PPCInstPrinter::getRegisterName(Reg);
> > > >
> > > > diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.h b/llvm/lib/Target/PowerPC/PPCInstrInfo.h
> > > > index ee16d43..ebd9024 100644
> > > > --- a/llvm/lib/Target/PowerPC/PPCInstrInfo.h
> > > > +++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.h
> > > > @@ -427,13 +427,17 @@ public:
> > > > /// to instructions that use both Altivec and VSX numbering (for different
> > > > /// operands).
> > > > static unsigned getRegNumForOperand(const MCInstrDesc &Desc, unsigned Reg,
> > > > - unsigned OpNo) {
> > > > + unsigned OpNo, bool equal = false) {
> > > > + unsigned oldReg = Reg;
> > > > if (Desc.TSFlags & PPCII::UseVSXReg) {
> > > > if (isVRRegister(Reg))
> > > > Reg = PPC::VSX32 + (Reg - PPC::V0);
> > > > else if (isVFRegister(Reg))
> > > > Reg = PPC::VSX32 + (Reg - PPC::VF0);
> > > > }
> > > > + if (equal) {
> > > > + assert(oldReg == Reg && "Reg is altered");
> > > > + }
> > > > return Reg;
> > > > }
> > > > };
> > > > ```
> > > > In contrast, if I keep it, for some inline assembly test case (e.g., CodeGen/Generic/2007-04-08-MultipleFrameIndices.ll), Desc.OpInfo would be null and I need additional logic to handle this case even the invocation is useless.
> > >
> > > Do we know why it works before but fail now?
> > >
> > > I apply the following patch and there is no failure in LIT/LNT tests.
> >
> >
> > Interesting. To double confirm, you applied the patch against ToT (without your proposed change of td files in this review), right? I am curious why we add this logic before?
> >
> > No failure in LIT/LNT tests does NOT prove that this is safe to me.
> > Can we get some testcase that are supposed to run into this logic (eg: some MI with UseVSXReg flags set) and check the 'Reg` values returned here? And compare to the `Reg` values with your proposed change?
> >
> For //CodeGen/Generic/2007-04-08-MultipleFrameIndices.ll// , `%A = call i32 asm sideeffect "inline asm $0 $2 $3 $4", "=r,0,i,m,m"( i32 0, i32 1, i8** %foo, i32** %bar )`, `MI->getOpcode()` is `INLINEASM` and `Desc.OpInfo` would be null. In original version, we don't use `Desc.OpInfo` at all.
Yes, I apply the patch mentioned above to the master (without my proposed change of td files in this review) and there is no failure.
'lxsdx 32, 5, 31' is the test case supposed to run into this logic (the instruction has UseVSXReg flag set). Here is the `Reg` values for the three operand:
```
echo 'lxsdx 32, 5, 31' | ./build/release/bin/llvm-mc --assemble -triple powerpc64-unknown-linux-gnu -mcpu=pwr7 test.s > /dev/null
oldReg: 183
Reg: 247
oldReg: 284
Reg: 284
oldReg: 310
Reg: 310
```
Both the master branch and the branch with my proposed change have the same result (I don't duplicate the output since they are the same).
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:896
def XXSPLTWs : XX2Form_2<60, 164,
- (outs vsrc:$XT), (ins vfrc:$XB, u2imm:$UIM),
+ (outs vsrc:$XT), (ins vsfrc:$XB, u2imm:$UIM),
"xxspltw $XT, $XB, $UIM", IIC_VecPerm, []>;
----------------
jsji wrote:
> Yi-Hong.Lyu wrote:
> > jsji wrote:
> > > Why we want to use `vsfrc` instead of `vfrc`? I think allowing `F8RC` may cause problem.
> > >
> > I think we can use `vsfrc` here since `xxspltw` is able to address whole 64 VSR registers. Besides, `XXSPLTWs` only uses second word of VSR register so there is no problem to use `F8RC`.
> > Besides, XXSPLTWs only uses second word of VSR register so there is no problem to use F8RC.
>
> Looks like to me that current patterns only use `1` for `$UIM` does not lead to conclusion that `$UIM` can only be `1`?
> What if I write a new pattern to use `XXSPLTWs, ... 3`?
>
>
The original used `vfrc` is `RegisterOperand<VFRC>`, `VFRC` consist of VF registers which are sub-register (i.e., first 64bit) of VSX32-VSX63. They don't work for `XXSPLTWs, ... 3` neither.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58685/new/
https://reviews.llvm.org/D58685
More information about the llvm-commits
mailing list