This is an archive of the discontinued LLVM Phabricator instance.

[FastISel][X86] Teach how to select SSE2/AVX bitcasts between 128/256-bit vector types.
ClosedPublic

Authored by andreadb on Oct 1 2015, 7:53 AM.

Details

Summary

While looking at the -O0 codegen, I noticed that X86FastIsel doesn't know how to fast select bitcasts between 128-bit vetor types.

This patch is a (very) minor FastISel improvement.
It teaches FastISel the following two things:

  1. On SSE2, no instructions are needed for bitcasts between 128-bit vector types.
  2. On AVX, no instructions are needed for bitcasts between 256-bit vector types.

Example:

%1 = bitcast <4 x i32> %V to <2 x i64>

Before (-fast-isel -fast-isel-abort=1):

FastIsel miss:  %1 = bitcast <4 x i32> %V to <2 x i64>

Now we don't fall back to SelectionDAG and we correctly fold that computation propagating the register associated to %V.

Please let me know if okay to submit.

Thanks,
Andrea

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb updated this revision to Diff 36242.Oct 1 2015, 7:53 AM
andreadb retitled this revision from to [FastISel][X86] Teach how to select SSE2/AVX bitcasts between 128/256-bit vector types..
andreadb updated this object.
andreadb added reviewers: qcolombet, ributzka, nadav.
andreadb added a subscriber: llvm-commits.
qcolombet edited edge metadata.Oct 1 2015, 2:00 PM

Hi Andrea,

Couple of questions, see inline, otherwise looks quite reasonable to me.

Thanks,
-Quentin

lib/Target/X86/X86FastISel.cpp
3240 ↗(On Diff #36242)

Why do we have to check that here?
We are interested in 128 and 256 bit vector types. I do not understand why the fact that f64 may be mapped on x87 need to be considered?

3247 ↗(On Diff #36242)

Could you add test cases for these checks?
In particular the different SizeInBits part.

Hi Quentin,

lib/Target/X86/X86FastISel.cpp
3240 ↗(On Diff #36242)

While it is true that X86ScalarSSEf64 is initialized to Subtarget->hasSSE2(), X86ScalarSSEf64 should only be used to reason on f64 types. So it is wrong to use it in this context.

I will change it to an explicit check for feature SSE2.

3247 ↗(On Diff #36242)

My bad.. That check makes no sense since source and destination bit widths must always be identical for bitcast operations.
I will remove that redundant check.

qcolombet added inline comments.Oct 1 2015, 2:44 PM
lib/Target/X86/X86FastISel.cpp
3247 ↗(On Diff #36242)

Hehe, thanks for checking, I was wondering if the data layout may break that assumption and if it does, when!

andreadb updated this revision to Diff 36308.Oct 1 2015, 3:01 PM
andreadb edited edge metadata.

Patch Updated.

Replaced the check for X86ScalarSSEf64 with a more appropriate check for SSE2.
Removed redundant check for type sizes.

Please let me know if okay to commit.
Thanks!

qcolombet accepted this revision.Oct 1 2015, 5:47 PM
qcolombet edited edge metadata.

Thanks Andrea!

LGTM.

Cheers,
-Quentin

This revision is now accepted and ready to land.Oct 1 2015, 5:47 PM
This revision was automatically updated to reflect the committed changes.