[PATCH] D58685: [NFC][PowerPC] Remove UseVSXReg

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

Yi-Hong.Lyu marked 3 inline comments as done.
Yi-Hong.Lyu added a comment.

I think using `f0` here is OK. Take another instruction `lxsdx` for example:

  $ echo '0x7c 0xe5 0xfc 0x98' | ./build/release/bin/llvm-mc --disassemble -triple powerpc64-unknown-linux-gnu -mcpu=pwr7 -ppc-asm-full-reg-names
          .text
          lxsdx f7, r5, r31

According to the spec, `lxsdx` will output to `VSX[XT]`. It uses `vsfrc` since it only uses first dword (`VSX[XT].dword[1]` is undefined). That's our intention to print `f7` instead of `vs7` here. Go back the this case, `lxsibzx` has the same situation as `lxsdx`. Besides, `XXSPLTWs` only uses in the pattern `xxspltw XT,XB, 1`[1] so it is well-defined to use `f0` here (`f0.word[1]` = `vs0.word[1]`).

[1]

  yihongl at kuat:/data/yihongl/dev/src/llvm/lib/Target/PowerPC$ grep -r XXSPLTWs
  P9InstrResources.td:    XXSPLTWs,
  PPCMIPeephole.cpp:          (MyOpcode == PPC::XXSPLTW && DefOpcode == PPC::XXSPLTWs) ||
  PPCInstrVSX.td:  def XXSPLTWs : XX2Form_2<60, 164,
  PPCInstrVSX.td:           (v4i32 (XXSPLTWs (LXSIBZX xoaddr:$src), 1))>;
  PPCInstrVSX.td:            (v4i32 (XXSPLTWs (VEXTSB2Ws (LXSIBZX xoaddr:$src)), 1))>;
  PPCInstrVSX.td:            (v4i32 (XXSPLTWs (LXSIHZX xoaddr:$src), 1))>;
  PPCInstrVSX.td:            (v4i32 (XXSPLTWs (VEXTSH2Ws (LXSIHZX xoaddr:$src)), 1))>;



================
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:
> 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;
   }
 };
```


================
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
----------------
jsji wrote:
> Can we add `default:`, even if it is unreachable?
`default:` is not unreachable because `regClass` might be `VRRCRegClassID`, `VFRCRegClassID` ... etc 


================
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:
> 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`.


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

https://reviews.llvm.org/D58685