[PATCH] D35408: [mips] Fix floating point select machine verifier errors

Simon Dardis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 19 09:15:04 PDT 2017


sdardis accepted this revision.
sdardis added a comment.
This revision is now accepted and ready to land.

LGTM with some (minor) nits addressed.



================
Comment at: lib/Target/Mips/Mips32r6InstrInfo.td:581-585
+def SDT_MipsFSelect : SDTypeProfile<1, 3, [SDTCisFP<1>,
+                                           SDTCisSameAs<0,2>,
+                                           SDTCisSameAs<2,3>]>;
+
+def MipsFSelect : SDNode<"MipsISD::FSELECT", SDT_MipsFSelect>;
----------------
Nit: Can you move this to the top of the file before the operand descriptions with the same section header as found in MipsInstrInfo.td? Also, add a section header for the operands.

I'd like to keep the file organised in a similar fashion to MipsInstrInfo.td. 


================
Comment at: lib/Target/Mips/MipsISelLowering.h:71
+      FSELECT,
+      MTC1_D64,
+
----------------
This move instruction requires it's own comment.


================
Comment at: lib/Target/Mips/MipsInstrFPU.td:42-43
 
+def SDT_MipsMTC1_D64 : SDTypeProfile<1, 1, [SDTCisVT<1, i32>,
+                                            SDTCisVT<0, f64>]>;
+
----------------
Nit, the textual order of the type of operands doesn't match the type declarations, i.e. this should be:

   def SDT_MipsMTC1_D64 : SDTypeProfile<1, 1, [SDTCisVT<0, f64>,
                                               SDTCisVT<1, i32>]>;



================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:372
+
+  if(!Subtarget.hasMips32r6() && !Subtarget.hasMips64r6())
+    return MipsTargetLowering::LowerOperation(Op, DAG);
----------------
if(!Subtarget.hasMips32r6()) is sufficient here as hasMips64r6() implies hasMips32r6().


================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:378
+
+  SDValue Tmp = DAG.getNode(MipsISD::MTC1_D64, DL, MVT::f64, Op->getOperand(0));
+  return DAG.getNode(MipsISD::FSELECT, DL, ResTy, Tmp, Op->getOperand(1),
----------------
Nit: add a comment explaining that although MTC1_D64 takes an i32 and writes an f64, the upper 32 bits of the floating point register are undefined. This isn't an issue as sel.d which is produced from an FSELECT node only looks at bit 0.


https://reviews.llvm.org/D35408





More information about the llvm-commits mailing list