Page MenuHomePhabricator

Implement floating point to integer conversion in mips fast-isel
ClosedPublic

Authored by rkotler on Oct 1 2014, 12:36 PM.

Details

Summary

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

Diff Detail

Event Timeline

rkotler updated this revision to Diff 14289.Oct 1 2014, 12:36 PM
rkotler retitled this revision from to Implement floating point to integer conversion in mips fast-isel.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler added subscribers: mcrosier, rfuhler, Unknown Object (MLST).

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

rkotler updated this revision to Diff 14290.Oct 1 2014, 1:43 PM

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 requested changes to this revision.Oct 3 2014, 2:38 AM
dsanders edited edge metadata.

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

This revision now requires changes to proceed.Oct 3 2014, 2:38 AM
dsanders added inline comments.Oct 3 2014, 5:54 AM
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 updated this revision to Diff 14475.Oct 6 2014, 2:43 PM
rkotler edited edge metadata.

Fix issues from last push request.

dsanders accepted this revision.Oct 8 2014, 8:31 AM
dsanders edited edge metadata.

LGTM with a couple new nits.

lib/Target/Mips/MipsFastISel.cpp
52–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

This revision is now accepted and ready to land.Oct 8 2014, 8:31 AM

> 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 updated this revision to Diff 14682.Oct 9 2014, 3:18 PM
rkotler edited edge metadata.

Remove comment on UnsupportedFPMode. Variable name is sufficient.

rkotler closed this revision.Oct 10 2014, 10:10 AM