Implement floating point to integer conversion in mips fast-isel

[rkotler]

Summary

Add the ability to convert 64 or 32 bit floating point values to integer in mips fast-isel

[rkotler]

Reran tests with latest resynch and got a make check failure. Am investigating.

[rkotler]

Previously had cleaned up make check test case and introduced an error from a typo. It's been fixed and make check is fine now.

[dsanders]

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'.

lib/Target/Mips/MipsFastISel.cpp

489–490

Style nit: Capitalization in the comment

519

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.

524

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().

test/CodeGen/Mips/Fast-ISel/fpintconv.ll

35–39

Style nit: Blank lines at EOF

[dsanders]

lib/Target/Mips/MipsFastISel.cpp

519

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.

524

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().

[rkotler]

Fix issues from last push request.

[dsanders]

LGTM with a couple new nits.

lib/Target/Mips/MipsFastISel.cpp

51–53

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.

489–490

Done

519

Done: FP64 guard added to this function. The rest will be done in a follow-up patch.

524

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.

test/CodeGen/Mips/Fast-ISel/fpintconv.ll

35–39

Done

[dsanders]

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.

I handled it this way because what the patch is really doing is to allow
fast-isel to work even in the context
of an unsupported fp mode; i.e. to let all other code be handled by
fast-isel and not disable the whole pass for that reason.

At this time, isFP64Bit is the only such mode but there can be others in
the future and the patch is not specific even to
fp64Bit but to the general problem of an unsupported fp mode.

The plan is even to support this new mode very soon.

I'm only commenting on the confusing wording of the comment. Perhaps it should say something like 'Used to disable floating point operations when in an unsupported FPU mode without affecting other operations'. Alternatively, we could delete the comment since 'UnsupportedFPMode' is quite self-explanatory.

[rkotler]

Remove comment on UnsupportedFPMode. Variable name is sufficient.

[dsanders]

LGTM

lib/Target/Mips/MipsFastISel.cpp

   bool SelectTrunc(const Instruction *I);
   bool SelectFPExt(const Instruction *I);
   bool SelectFPTrunc(const Instruction *I);
+  bool SelectFPToI(const Instruction *I, bool IsSigned);
 
   bool isTypeLegal(Type *Ty, MVT &VT);
   bool isLoadTypeLegal(Type *Ty, MVT &VT);

   return true;
 }
 
+// Attempt to fast-select a floating-point-to-integer conversion.
+bool MipsFastISel::SelectFPToI(const Instruction *I, bool IsSigned) {
+  MVT DstVT, SrcVT;
+  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();
+  if (!isTypeLegal(DstTy, DstVT))
+    return false;
+
+  if (DstVT != MVT::i32)
+    return false;
+
+  Value *Src = I->getOperand(0);
+  Type *SrcTy = Src->getType();
+  if (!isTypeLegal(SrcTy, SrcVT))
+    return false;
+
+  if (SrcVT != MVT::f32 && SrcVT != MVT::f64)
+    return false;
+
+  unsigned SrcReg = getRegForValue(Src);
+  if (SrcReg == 0)
+    return false;
+
+  // Determine the opcode for the conversion, which takes place
+  // entirely within FPRs.
+  unsigned DestReg = createResultReg(&Mips::GPR32RegClass);
+  unsigned TempReg = createResultReg(&Mips::FGR32RegClass);
+  unsigned Opc;
+
+  if (SrcVT == MVT::f32)
+    Opc = Mips::TRUNC_W_S;
+  else
+    Opc = Mips::TRUNC_W_D32;
+
+  // Generate the convert.
+  EmitInst(Opc, TempReg).addReg(SrcReg);
+
+  EmitInst(Mips::MFC1, DestReg).addReg(TempReg);
+
+  updateValueMap(I, DestReg);
+  return true;
+}
+
 bool MipsFastISel::fastSelectInstruction(const Instruction *I) {
   if (!TargetSupported)
     return false;

     return SelectFPTrunc(I);
   case Instruction::FPExt:
     return SelectFPExt(I);
+  case Instruction::FPToSI:
+    return SelectFPToI(I, /*isSigned*/ true);
+  case Instruction::FPToUI:
+    return SelectFPToI(I, /*isSigned*/ false);
   }
   return false;
 }

test/CodeGen/Mips/Fast-ISel/fpintconv.ll

+; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
+; RUN:     < %s | FileCheck %s
+; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32 \
+; RUN:     < %s | FileCheck %s
+
+
+@f = global float 0x40D6E83280000000, align 4
+@d = global double 0x4132D68780000000, align 8
+@i_f = common global i32 0, align 4
+@i_d = common global i32 0, align 4
+@.str = private unnamed_addr constant [5 x i8] c"%i \0A\00", align 1
+
+; Function Attrs: nounwind
+define void @ifv() {
+entry:
+; CHECK-LABEL:   .ent  ifv
+  %0 = load float* @f, align 4
+  %conv = fptosi float %0 to i32
+; CHECK:   trunc.w.s  $f{{[0-9]+}}, $f{{[0-9]+}}
+  store i32 %conv, i32* @i_f, align 4
+  ret void
+}
+
+; Function Attrs: nounwind
+define void @idv() entry:
+; CHECK-LABEL:   .ent  idv
+  %0 = load double* @d, align 8
+  %conv = fptosi double %0 to i32
+; CHECK:   trunc.w.d  $f{{[0-9]+}}, $f{{[0-9]+}}
+  store i32 %conv, i32* @i_d, align 4
+  ret void
+}
+
+
+
+
+