[PATCH] Implement floating point to integer conversion in mips fast-isel

[Daniel Sanders]

LGTM with a couple new nits.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:489-490
@@ +488,4 @@
+  if (!IsSigned)
+    return false; // we don't handle this case yet. there is no native
+                  // instruction for this but it can be synthesized.
+  Type *DstTy = I->getType();
----------------
dsanders wrote:
> Style nit: Capitalization in the comment
Done

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:519
@@ +518,3 @@
+  else
+    Opc = Mips::TRUNC_W_D32;
+
----------------
dsanders wrote:
> dsanders wrote:
> > This is only the correct for 32-bit FPU's and we don't have any isFP64bit() checks. 64-bit FPU's must use TRUNC_W_D64.
> > 
> > Please fix this to handle a 64-bit FPU or add a guard against it.
> > 
> > I looked for other instances of this that we missed and found a few:
> > * In EmitLoad: LDC1 must be LDC164 for the FP64 case (and the register it defines needs to be a FGR64)
> > * In EmitStore: SDC1 must be SDC164 for the FP64 case
> > * In SelectFPExt: CVT_D32_S must be CVT_D64_S for the FP64 case (and the register it defines needs to be a FGR64)
> > * In SelectFPTrunc: CVT_S_D32 must be CVT_S_D64 for the FP64 case
> > * In MaterializeFP: BuildPairF64 must be BuildPairF64_64 for the FP64 case (and the register it defines needs to be a FGR64)
> > 
> > Please fix these in a follow up patch.
> > Mostly this can all be fixed with the isFP64bit() guard in the supported target section.
> 
> That will work but I'd prefer the checks to be added where the code depends on the value for two reasons. The first is that we'll have to move the checks to these places when we come to support FP64 anyway. The second is that it doesn't make sense to have to fall back on SelectionDAG for operations that don't involve the FPU (e.g. integer addition) just because the FPU is in 64-bit mode.
Done: FP64 guard added to this function. The rest will be done in a follow-up patch.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:524
@@ +523,3 @@
+
+  EmitInst(Mips::MFC1, DestReg).addReg(TempReg);
+
----------------
dsanders wrote:
> dsanders wrote:
> > I prefer using MFC1 directly if it works but there's an awkward bug in the definition of MFC1 that allows the reordering of MFHC1/MFC1 sequences even though this changes the behaviour of the code. SelectionDAG works around it using ExtractElementF64 (for FP32) and ExtractElementF64_64 (for FP64).
> > 
> > The detail is in MipsSEInstrInfo::expandExtractElementF64(). Here's the relevant comment from that function:
> >     // FIXME: Strictly speaking MFHC1 only reads the top 32-bits however, we      
> >     //        claim to read the whole 64-bits as part of a white lie used to      
> >     //        temporarily work around a widespread bug in the -mfp64 support.     
> >     //        The problem is that none of the 32-bit fpu ops mention the fact     
> >     //        that they clobber the upper 32-bits of the 64-bit FPR. Fixing that  
> >     //        requires a major overhaul of the FPU implementation which can't     
> >     //        be done right now due to time constraints.                          
> >     //        MFHC1 is one of two instructions that are affected since they are   
> >     //        the only instructions that don't read the lower 32-bits.            
> >     //        We therefore pretend that it reads the bottom 32-bits to            
> >     //        artificially create a dependency and prevent the scheduler          
> >     //        changing the behaviour of the code.
> > and there's a similar comment regarding MTC1/MTHC1 in MipsSEInstrInfo::expandBuildPairF64().
> > Was not sure exactly what you wanted me to do for this MFC1
> > If I look at addExtractElementF64 there is an alternate sequence that I can use from that?
> 
> I'd like you to either confirm that the bug can't occur for Fast ISel or use ExtractElementF64/ExtractElementF64_64 instead of MFC1 so that we use the workaround from expandExtractElementF64().
I've gone through the code in the SelectionDAG implementation and it turns out this is ok as is. expandExtractElementF64 does not apply any workarounds to the case where the low 32-bits are extracted. It only manipulates the case where the high 32-bits are extracted.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:51-53
@@ -50,3 +50,5 @@
   bool TargetSupported;
-
+  bool UnsupportedFPMode; // To allow fast-isel to proceed and just not handle
+  // floating point but not reject doing fast-isel in other
+  // situations
 public:
----------------
This comment needs some clarification. It's only 64-bit FPU's you're guarding against but the comment suggests that it's floating point in general.

Also, indentation.

================
Comment at: test/CodeGen/Mips/Fast-ISel/fpintconv.ll:34-38
@@ +33,6 @@
+}
+
+
+
+
+
----------------
dsanders wrote:
> Style nit: Blank lines at EOF
Done

http://reviews.llvm.org/D5562