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

[Reed Kotler]

Mostly this can all be fixed with the isFP64bit() guard in the supported target section.

Is that correct?

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?


________________________________________
From: Daniel Sanders
Sent: Friday, October 03, 2014 2:38 AM
To: Reed Kotler; Daniel Sanders
Cc: llvm-commits at cs.uiuc.edu; Rich Fuhler; mcrosier at codeaurora.org
Subject: Re: [PATCH] Implement floating point to integer conversion in mips fast-isel

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