[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