Add the ability to convert 64 or 32 bit floating point values to integer 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'.
Style nit: Capitalization in the comment
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:
Please fix these in a follow up patch.
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().
Style nit: Blank lines at EOF
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.
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().
LGTM with a couple new nits.
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.
Done: FP64 guard added to this function. The rest will be done in a follow-up patch.
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.
> 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
> + floating point but not reject doing fast-isel in other
> + // situations
> 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.