This is an archive of the discontinued LLVM Phabricator instance.

[x86] allow 128/256-bit lowering for copysign vector intrinsics (PR30433)
ClosedPublic

Authored by spatel on Oct 2 2016, 10:52 AM.

Diff Detail

Event Timeline

spatel updated this revision to Diff 73219.Oct 2 2016, 10:52 AM
spatel retitled this revision from to [x86] allow 128/256-bit lowering for copysign vector intrinsics (PR30433).
spatel updated this object.
spatel added reviewers: RKSimon, zvi, craig.topper.
spatel added a subscriber: llvm-commits.
zvi edited edge metadata.Oct 2 2016, 3:35 PM

LGTM.
Note that since sign-mask and mag-mask are complementary, we could avoid one constant-pool load with ANDNPS.

spatel added a comment.Oct 3 2016, 7:07 AM
In D25165#558579, @zvi wrote:

LGTM.
Note that since sign-mask and mag-mask are complementary, we could avoid one constant-pool load with ANDNPS.

Thanks, Zvi. I thought about that, but this is a general problem: AFAIK, we just don't have very good constant combining/hoisting logic in the backend. I noticed this recently in:
https://llvm.org/bugs/show_bug.cgi?id=28672#c3

Also, see:
https://llvm.org/bugs/show_bug.cgi?id=25554
https://llvm.org/bugs/show_bug.cgi?id=24448
https://llvm.org/bugs/show_bug.cgi?id=27202

It seems like constant combining at the machine level could be its own pass, so we shouldn't have to special-case situations like this one.

spatel updated this revision to Diff 73294.Oct 3 2016, 9:32 AM
spatel edited edge metadata.

Patch updated:
Simon added cost model and SLP vectorizer tests that are affected by the new legalization parameters. I scanned those diffs, and they all look like improvements, but some adjustments may be warranted to fine-tune the cost model.

spatel accepted this revision.Oct 3 2016, 2:14 PM
spatel added a reviewer: spatel.

Phab isn't working like it used to - reclassifying as 'accepted' again.

This revision is now accepted and ready to land.Oct 3 2016, 2:14 PM