[PATCH] D34276: [mips] Alter register classes for MSA pseudo f16 instructions

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

The conversions introduced by this patch are incorrect. fexdo.[hw] and fexupr.[wd] implement the necessary down and up scaling of a floating point value.

The other nit I see if that you're creating virtual registers unconditionally, ideally they should only be created when they are going to be used.



================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3596-3604
+  unsigned Tmp2 = RegInfo.createVirtualRegister(&Mips::GPR64RegClass);
+  BuildMI(*BB, MI, DL, TII->get(Mips::COPY_U_H), !UsingMips32? Tmp : Rs)
+      .addReg(Ws)
+      .addImm(0);
+  if(!UsingMips32)
+    BuildMI(*BB, MI, DL, TII->get(Mips::SUBREG_TO_REG), Tmp2)
+        .addImm(0)
----------------
Rather than unconditionally allocating a virtual GPR64 register, you should instead have the output of COPY_U_H go to Rs, then if !UsingMips32, allocate a GPR64 and perform the SUBREG_TO_REG to writing the temp register, then assign the value of the temp register to Rs.

Then you don't need the conditional on line 3606.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3654-3656
+  unsigned Tmp = RegInfo.createVirtualRegister(&Mips::GPR32RegClass);
+  if(!UsingMips32) 
+    BuildMI(*BB, MI, DL, TII->get(Mips::COPY), Tmp).addReg(Rt, 0, Mips::sub_32);
----------------
You can sink the creation of the virtual register into the if block, along with "Rt = Tmp;" after emitting the copy.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3742-3745
+  unsigned Rtemp1 = RegInfo.createVirtualRegister(&Mips::FGR32RegClass);  
+  if (IsFGR64onMips32) 
+    BuildMI(*BB,MI,DL,TII->get(Mips::CVT_S_D64),Rtemp1).addReg(Fs);
+  BuildMI(*BB, MI, DL, TII->get(MFC1Opc), Rtemp).addReg(IsFGR64onMips32 ? Rtemp1: Fs);
----------------
This is incorrect as it's inconsistent with the logic below.

If the input is a FPR64 on Mips32, this patch changes the expansion to convert a double precision value to a single precision value. This value is then written to all the word lanes.

Lines 3749 - 3763 will write the upper 32bits of the original double into the odd lanes which result in incorrect values in the register when we perform FEXDO_W as that instruction expects doubles in the vector register.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3869-3871
+    BuildMI(*BB,MI,DL,TII->get(Mips::CVT_D64_S),Rtemp3).addReg(FPRPHI);
     BuildMI(*BB, MI, DL, TII->get(Mips::MTHC1_D64), Fd)
+        .addReg(Rtemp3)
----------------
This is incorrect, the input to the conversion is not single precision value, and it's been overwritten by the MTHC1.


https://reviews.llvm.org/D34276