Page MenuHomePhabricator

[InstCombine] Make (X|C1)^C2 -> X^(C1^C2) iff X&~C1 == 0 work for splat vectors
ClosedPublic

Authored by craig.topper on Aug 8 2017, 10:09 PM.

Details

Summary

This also corrects the description to match what was actually implemented. The old comment said X^(C1|C2), but it implemented X^((C1|C2)&~(C1&C2)). I believe ((C1|C2)&~(C1&C2)) is equivalent to (C1^C2).

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Aug 8 2017, 10:09 PM
spatel accepted this revision.Aug 10 2017, 7:23 AM

LGTM.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2393–2394 ↗(On Diff #110326)

Nit / potential clean-up: I like to make the variable names match the comment names for easier reading when possible, so I'd pick either 'X' or 'V' and stick with it throughout this block.

test/Transforms/InstCombine/select-with-bitwise-ops.ll
271 ↗(On Diff #110326)

Are we just changing the mask constant in these 3 tests to prove that MaskedValueIsZero() is working as expected? Might be more interesting to use a shift or zext to clear some bits instead?

This revision is now accepted and ready to land.Aug 10 2017, 7:23 AM
craig.topper added inline comments.Aug 10 2017, 9:25 AM
test/Transforms/InstCombine/select-with-bitwise-ops.ll
271 ↗(On Diff #110326)

These are all tests for select combines.

Test65 tests an and of a larger type with a power of 2 constant that fits in the smaller type. Until ver recently we tried to truncate first then shift. So it only worked for the smaller constant.
Test66 tests an and of a larger type with a power of 2 constant that does not fit in the smaller type. Until recently we punted this transform because we wanted to truncate first. But that would have cut off the constant.
Test67 tests an and of a smaller type with a power of 2 constant.

I don't think these tests were intentionally trying to test the xor combine we have here. It just happens that they trigger it. But the fact that they trigger it and the vector versions didn't is why I even noticed in the first place.

This revision was automatically updated to reflect the committed changes.