This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix sitofp and uitofp instruction matching failures with long double and avx512
ClosedPublic

Authored by mbodart on Aug 26 2015, 11:00 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

mbodart updated this revision to Diff 33220.Aug 26 2015, 11:00 AM
mbodart retitled this revision from to [X86] Fix sitofp and uitofp instruction matching failures with long double and avx512.
mbodart updated this object.
mbodart added reviewers: mkuper, nadav.
mbodart added a subscriber: llvm-commits.
AsafBadouh added inline comments.
lib/Target/X86/X86ISelLowering.cpp
1332 ↗(On Diff #33220)

I'm not sure, but I think you missed SINT_TO_FP case.

mbodart added inline comments.Sep 1 2015, 9:23 AM
lib/Target/X86/X86ISelLowering.cpp
1332 ↗(On Diff #33220)

Hi Asaf,

The SINT_TO_FP case does not require as many changes as the UINT_TO_FP case.

As the signed conversion instructions [v]cvtsi2ss/sd have existed since SSE/SSE2, LowerSINT_TO_FP already properly handles i32 and i64. And the operation action is properly set to Custom earlier in this function.

But the unsigned conversion instructions vcvtusi2ss/sd are new as of avx512f, so support for i32/i64->FP needed to be added to LowerUINT_TO_FP, and their operation action needed to be changed to Custom above.

regards,
Mitch

mkuper edited edge metadata.Sep 2 2015, 3:53 AM

Thanks, Mitch!
A couple of comments inline.

lib/Target/X86/X86ISelLowering.cpp
167 ↗(On Diff #33220)

Any way to re-factor the if chain so we don't end up with two checks for useSoftFloat()? Or does that end up being uglier?

1332 ↗(On Diff #33220)

I think what Asaf meant was that you removed setting SINT_TO_FP to Legal here, but didn't re-introduce it anywhere else. Does it get set to Legal elsewhere/by default?

12258 ↗(On Diff #33220)

Do we only get here for f32/f64? If not, do you need to check the DstTy here is not f80? If yes, perhaps make the comment a bit clearer to emphasize that is the case?

mbodart added inline comments.Sep 2 2015, 9:44 AM
lib/Target/X86/X86ISelLowering.cpp
167 ↗(On Diff #33220)

I think it would be about a wash. But there's a separate issue there, where UINT_TO_FP MVT::i64 is currently set to custom under is64Bit, independent of soft float. Retaining that behavior would make using a single soft float check a bit uglier, so is not something I want to mess with in this change set.

1332 ↗(On Diff #33220)

It already gets set to Custom earlier in the routine.
The SINT setOperationAction is not present in the Phabricator view because
it's not near enough to the UINT changes.

For a conversion that has a mix of Legal and Custom lowerings
depending on the operand/result types, the legal cases are then handled
by letting Lower[US]INT_TO_FP be a no-op, just returning the original
operation unmodified. This was already happening correctly in the SINT case.

BTW, for additional testing I wrote small functions exercising all integer and fp
data types, compiled with llc across various triples and subtargets, and compared
the assembly output. The only changes were that the f80 cases under avx512f now
compile successfully (and produce reasonable code).

12258 ↗(On Diff #33220)

f80 can reach here, but is filtered by the isScalarFPTypeInSSEReg check.
Though perhaps not obvious, this seems to be a commonly used check in this file
to distinguish between FP types in SSE registers (which must be f32 or f64), versus
FP types in X87 (like f80, but also f32/f64 under no-sse).

So it would be incomplete to single out just f80. I think the comment is
consistent with other uses of isScalarFPTypeInSSEReg in this file, so would
prefer to keep it as is.

mkuper accepted this revision.Sep 16 2015, 5:16 AM
mkuper edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 16 2015, 5:16 AM
This revision was automatically updated to reflect the committed changes.