This is an archive of the discontinued LLVM Phabricator instance.

[X86][SSE] Vector integer/float conversion memory folding (cvttps2dq / cvttpd2dq)
ClosedPublic

Authored by RKSimon on Oct 27 2014, 8:23 AM.

Details

Summary

Split from http://reviews.llvm.org/D5981

Fixed an issue with the VCVTTPD2DQ / VCVTTPS2DQ instructions being incorrectly put in the 2 source operand folding tables instead of the 1 source operand and added the missing 256-bit AVX versions

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 15493.Oct 27 2014, 8:23 AM
RKSimon retitled this revision from to Vector integer/float conversion memory folding (cvttps2dq / cvttpd2dq).
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon added reviewers: qcolombet, spatel.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
RKSimon retitled this revision from Vector integer/float conversion memory folding (cvttps2dq / cvttpd2dq) to [X86][SSE] Vector integer/float conversion memory folding (cvttps2dq / cvttpd2dq).Oct 27 2014, 11:44 AM
qcolombet edited edge metadata.Oct 27 2014, 12:20 PM

Hi Simon,

Thanks for having split the patches.

See my comments inlined.

Cheers,
-Quentin

lib/Target/X86/X86InstrInfo.cpp
936 ↗(On Diff #15493)

While you are fixing this kind of issue, could you double check the opcode in there?
All the CVTs look suspicious to me.

test/CodeGen/X86/avx1-stack-reload-folding.ll
22 ↗(On Diff #15493)

Could you trigger the transformation with something simpler (like load, cvt, store, with both addresses as argument)?
Maybe by using fast-isel?

RKSimon updated this revision to Diff 15859.Nov 6 2014, 5:34 AM
RKSimon edited edge metadata.

Updated the patch to follow the test pattern used in http://reviews.llvm.org/D5981

Added the (v)cvtps2dq / (v)cvtpd2dq folds as well

Many of the Int_CVT and CVT 'scalar' folds look suspicious and I wish to more thoroughly test them, but would prefer to do that separately from this patch.

I found some rather poor code generation for non-AVX code for double -> int32 that needs fixing as well.

qcolombet accepted this revision.Nov 6 2014, 1:32 PM
qcolombet edited edge metadata.

Many of the Int_CVT and CVT 'scalar' folds look suspicious and I wish to more thoroughly test them, but would prefer to do that separately from this patch.

Sounds good to me.

Please add some CHECK-LABELs for the functions you added for testing.
With that, LGTM (no need to send an updated review).

Thanks,
Q.

This revision is now accepted and ready to land.Nov 6 2014, 1:32 PM
RKSimon closed this revision.Nov 6 2014, 2:26 PM
RKSimon updated this revision to Diff 15893.

Closed by commit rL221489 (authored by @RKSimon).