[PATCH] D58685: [PowerPC] Remove UseVSXReg

[Yi-Hong Lyu via Phabricator via llvm-commits]

Yi-Hong.Lyu marked 4 inline comments as done.
Yi-Hong.Lyu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:167
   case MachineOperand::MO_Register: {
-    unsigned Reg = PPCInstrInfo::getRegNumForOperand(MI->getDesc(),
-                                                     MO.getReg(), OpNo);
----------------
Yi-Hong.Lyu wrote:
> 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).
@nemanjai  Could you give some insight that why we need invoke `getRegNumForOperand()` here? I cannot come up with a test case that use the mapping in that function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58685/new/

https://reviews.llvm.org/D58685