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

Daniel Sanders daniel.sanders at imgtec.com
Fri Oct 3 02:38:13 PDT 2014


When uploading patches via the web interface, please include the full context by creating your patch with 'git diff -U999999 branch' or 'svn diff --diff-cmd=diff -x -U999999'.

================
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();
----------------
Style nit: Capitalization in the comment

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:519
@@ +518,3 @@
+  else
+    Opc = Mips::TRUNC_W_D32;
+
----------------
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.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:524
@@ +523,3 @@
+
+  EmitInst(Mips::MFC1, DestReg).addReg(TempReg);
+
----------------
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().

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

http://reviews.llvm.org/D5562






More information about the llvm-commits mailing list