[PATCH] D26398: [mips][msa] Implement f16 support

[Simon Dardis via llvm-commits]

sdardis planned changes to this revision.
sdardis marked 6 inline comments as done.
sdardis added a comment.

As I mention in my inline comments, I'm revising this patch. I'll highlight the differences when I repost.



================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3448
+
+  const bool IsN64 = Subtarget.isABI_N64();
+  const TargetRegisterClass *RC =
----------------
zoran.jovanovic wrote:
> Nit: I do believe that Mips::GPR64RegClass and 64-bit variants of instructions should be used with N32 too.
> Same is for code in MipsSETargetLowering::emitLD_F16_PSEUDO.
Looking at this, I have found test cases where N32 requires either GPR32RegClass or GPR64RegClass depending on the context. I'll post a revised version of this patch soon. I've also found 1 or 2 places where the wrong MSA regclass is used.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3655
+// =>
+//  fexupr.w $wtemp, $ws
+//  copy_s.w $rtemp, $wtemp[0]
----------------
zoran.jovanovic wrote:
> Nit: Is this comment correct? 
> From the code below (and corresponding test cases) it seems that fexupr.d instruction should be generated after this one.
> 
Yes, you're correct, I missed that when writing the comment. Probably a copy/paste thing.


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:3689
+  BuildMI(*BB, MI, DL, TII->get(Mips::FEXUPR_W), Wtemp).addReg(Ws);
+  if (IsFGR64) {
+    WPHI = RegInfo.createVirtualRegister(&Mips::MSA128WRegClass);
----------------
zoran.jovanovic wrote:
> if comment above is correct IsFGR64 should be replaced with IsFGR64onMips64.
(For completeness' sake) The comment above is wrong, If we're expanding an f16 to FPR64, we need FEXUPRD as well. The case of Mips32/Mips64 doesn't apply here.


Repository:
  rL LLVM

https://reviews.llvm.org/D26398