Page MenuHomePhabricator

[X86][FastIsel] Teach how to select scalar integer to float/double conversions.
ClosedPublic

Authored by andreadb on Feb 17 2015, 5:05 AM.

Details

Summary

This patch teaches fast-isel how to handle integer to float/double conversions.
In particular, this patch teaches fast-isel how to select a (V)CVTSI2SSrr for an integer to float conversion, and how to select (V)CVTSI2SDrr for an integer to double conversion.

Added test 'fast-isel-int-float-conversion.ll'.

Please let me know if ok to submit.

Thanks,
Andrea

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb updated this revision to Diff 20079.Feb 17 2015, 5:05 AM
andreadb retitled this revision from to [X86][FastIsel] Teach how to select scalar integer to float/double conversions..
andreadb updated this object.
andreadb edited the test plan for this revision. (Show Details)
andreadb added reviewers: mkuper, ributzka, qcolombet.
andreadb added a subscriber: Unknown Object (MLST).
qcolombet edited edge metadata.Feb 17 2015, 10:08 AM

Hi Andrea,

The code looks mostly good to me, but I am confused by the tests.
Shouldn’t we match some cvt instructions for the SSE2 pattern?

Thanks,
-Quentin

lib/Target/X86/X86FastISel.cpp
2010 ↗(On Diff #20079)

Use early exist here.

test/CodeGen/X86/fast-isel-int-float-conversion.ll
7 ↗(On Diff #20079)

Shouldn’t we have some check for the actual pattern for SSE2?

Hi Quentin,

lib/Target/X86/X86FastISel.cpp
2010 ↗(On Diff #20079)

I will use an early-exit.

test/CodeGen/X86/fast-isel-int-float-conversion.ll
7 ↗(On Diff #20079)

Yes, for SSE2 we could match 'cvtsi2sdl %edi, %xmm0'. I just wanted to make sure that with SSE2 we didn't have the vex prefix. I agree that it is a bit confusing so I will change it.

andreadb updated this revision to Diff 20097.Feb 17 2015, 10:33 AM
andreadb edited edge metadata.

Hi Quentin,

Here is an updated patch.
As you suggested, I added an early exit in X86SelectSIToFP. I also fixed the check lines for SSE2.

Please let me know what you think.

Thanks in advance,
Andrea

ributzka added inline comments.Feb 17 2015, 10:40 AM
lib/Target/X86/X86FastISel.cpp
2046 ↗(On Diff #20097)

You may want to constrain the register class with "constrainOperandRegClass".

qcolombet accepted this revision.Feb 17 2015, 10:48 AM
qcolombet edited edge metadata.

Thanks Andrea!

LGTM with Juergen’s suggestion. I believe this is not needed for correctness, but it is definitely cleaner!

Cheers,
-Quentin

test/CodeGen/X86/fast-isel-int-float-conversion.ll
7 ↗(On Diff #20097)

You could still keep the SSE2-NOT with vcvt stuff if you want to be sure :).

This revision is now accepted and ready to land.Feb 17 2015, 10:48 AM
andreadb updated this revision to Diff 20100.Feb 17 2015, 12:09 PM
andreadb edited edge metadata.

Hi Quentin, Juergen

thanks a lot for the reviews!
So, I have added a call to 'constrainOperandRegClass' to ensure that 'OpReg' is in the correct register class.
Since I am not very familiar with that method, before committing I just wanted to double-check with you if this new version of the patch is ok to commit :-).

Thanks again for your time!
Andrea

qcolombet added inline comments.Feb 17 2015, 1:57 PM
lib/Target/X86/X86FastISel.cpp
2042 ↗(On Diff #20100)

Should be just 1 instead of (HasAVX …).

Indeed, OpReg is the equivalent of the first operand for the description of the CVT. The fact that you will insert an implicit def is orthogonal so you do not have to account for that when doing the query.

Hi Quentin,

lib/Target/X86/X86FastISel.cpp
2042 ↗(On Diff #20100)

I tried to just pass 1 instead of checking if the target HasAVX. However, the code generated is wrong..
For example, with that change, I get the following assembly:

int_to_double_rr:

vmovd   %edi, %xmm1
vcvtsi2sd       %xmm1, %xmm0, %xmm0

int_to_float_rr:

vmovd   %edi, %xmm1
vcvtsi2ssl      %xmm1, %xmm0, %xmm0
retq

The reason why I added a check for AVX is because OpReg is expected to be the last input operand. According to X86InstrSSE.td, OpReg should be the second operand for the AVX variant. If I pass index 2 on AVX, then I get the correct results. This seems to suggest that we have to account for the IMPLICIT_DEF when doing the query.

Please commit!

lib/Target/X86/X86FastISel.cpp
2042 ↗(On Diff #20100)

Indeed!

I forgot that the AVX variant copied the first operand.

Sorry for the noise!

This revision was automatically updated to reflect the committed changes.

Hi Andrea,

On a second thought, why do we need to use the AVX variant at all if we never use the first source operand?
The bonus point is that would kill all the code related to the implicit def.

Thanks,
-Quentin

Hi Andrea,

On a second thought, why do we need to use the AVX variant at all if we never use the first source operand?
The bonus point is that would kill all the code related to the implicit def.

Hi Quentin,

The main reason (at least from my point of view) is that we want to avoid mixing legacy SSE instructions and AVX instructions as much as possible. Otherwise, we end up increasing AVX-SSE transition penalties. That's why (basically everywhere in X86FastISel) we always prefer vex encoded instructions over legacy SSE instructions.

I hope this makes sense.
-Andrea

Yeah, that makes sense.
I was just wondering if that specific instruction had such a penalty.

Thanks for double checking!

Quentin