This is an archive of the discontinued LLVM Phabricator instance.

[x86, AVX] allow FP vector select folding to bitwise logic ops (PR28895)
ClosedPublic

Authored by spatel on Aug 9 2016, 3:58 PM.

Details

Summary

This handles the case in:
https://llvm.org/bugs/show_bug.cgi?id=28895

...but I suspect that we may not be getting all of the possibilities yet. Eg, we can use 'X86::FANDN' for scalar FP select combines.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 67428.Aug 9 2016, 3:58 PM
spatel retitled this revision from to [x86, AVX] allow FP vector select folding to bitwise logic ops (PR28895).
spatel updated this object.
spatel added reviewers: delena, RKSimon, andreadb.
spatel added a subscriber: llvm-commits.
delena accepted this revision.Aug 10 2016, 12:41 AM
delena edited edge metadata.
This revision is now accepted and ready to land.Aug 10 2016, 12:41 AM
andreadb accepted this revision.Aug 10 2016, 1:35 AM
andreadb edited edge metadata.

Hi Simon,

It looks good to me too.
Could you please also add extra test cased with ordered comparisons? I am only seeing tests for unordered comparisons in 'select-with-and-or.ll'.

Thanks for fixing this!
-Andrea

Could you please also add extra test cased with ordered comparisons? I am only seeing tests for unordered comparisons in 'select-with-and-or.ll'.

Good point - I lazily copied the existing tests here but only changed the select operands to FP types. I changed the tests to use varying predicates in:
rL278229

Let me know if you see any holes in the coverage - fun mental exercise of translating LLVM predicates to SSE predicates... :)
If there is a bug in that part, it should be independent of this patch.

Could you please also add extra test cased with ordered comparisons? I am only seeing tests for unordered comparisons in 'select-with-and-or.ll'.

Good point - I lazily copied the existing tests here but only changed the select operands to FP types. I changed the tests to use varying predicates in:
rL278229

Let me know if you see any holes in the coverage - fun mental exercise of translating LLVM predicates to SSE predicates... :)
If there is a bug in that part, it should be independent of this patch.

Hi Sanjay,

The test coverage seems OK.
If you really want to increase the test coverage, then you could probably add a test for fcmp ole.

Something like this:

;;;;
define <4 x float> @test_ole(<4 x float> %A, <4 x float> %B, <4 x float> %C) {
entry:

%0 = fcmp ole <4 x float> %A, %B
%1 = select <4 x i1> %0, <4 x float> %C, <4 x float> zeroinitializer
ret <4 x float> %1

}
;;;;

That said, your patch does the right thing.
For the example above, I verified that the compiler now generates a vcmpleps + vandps.

So, the patch lgtm. Thanks!

This revision was automatically updated to reflect the committed changes.