[PATCH] D58685: [NFC][PowerPC] Remove UseVSXReg
Jinsong Ji via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Mar 3 21:04:30 PST 2019
jsji added a comment.
Thanks for addressing comments, however, I am still not convinced that this is safe.
I am also curious how this will pass machine verification?
Can you please try to run machine verification to all LIT/LNT test to see whether there is any register class mismatch due to this? Thanks.
================
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:
> > 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?
================
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:
> 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?
================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:430
+ switch (regClass) {
+ // We store F0-F31, VF0-VF31 in MCOperand and it should be F0-F31,
+ // VSX32-VSX63 during encoding/disassembling
----------------
Yi-Hong.Lyu wrote:
> jsji wrote:
> > Can we add `default:`, even if it is unreachable?
> `default:` is not unreachable because `regClass` might be `VRRCRegClassID`, `VFRCRegClassID` ... etc
then can we add a `default:` fall through with comments?
================
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, []>;
----------------
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`?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58685/new/
https://reviews.llvm.org/D58685
More information about the llvm-commits
mailing list