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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

I like the idea of trying to use register class to avoid using UseVSXReg flag.

However, **are we should this will work? **

Mixing FRC and VSRC may cause unexpected failures, or  at least causing confusion,

eg: If we add `-ppc-asm-full-reg-names`, then `lxsibzx 0, 0, 3` in `vsx-partword-int-loads-and-stores.ll`  in the patch will show up like:

  lxsibzx f0, 0, r3
  xxspltw vs34, f0, 1

Why we use `f0` to VSX instruction lxsibzx? 
This is confusing and may cause problem! 
Although `f0` is mapped to `vs0`, but they are still different: `f0` is only half of `vs0`.

BTW: You should not mark this as NFC , you have changed register classes, there ARE functional changes!



================
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());
 
----------------
Are we sure we don't need mapping any more?


================
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
----------------
Can we add `default:`, even if it is unreachable?


================
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, []>;
----------------
Why we want to use `vsfrc` instead of `vfrc`? I think allowing `F8RC` may cause problem.



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

https://reviews.llvm.org/D58685