Page MenuHomePhabricator

[InstCombine] Canonicalize variable mask in masked merge
ClosedPublic

Authored by lebedev.ri on Apr 15 2018, 3:08 AM.

Details

Summary

Masked merge has a pattern of: ((x ^ y) & M) ^ y.
But, there is no difference between ((x ^ y) & M) ^ y and ((x ^ y) & ~M) ^ x,
We should canonicalize it and drop that xor.

https://rise4fun.com/Alive/Yol

Diff Detail

Repository
rL LLVM

Event Timeline

Slightly cleanup the matcher by using m_CombineAnd(),
which allows to use m_c_And() when looking for A part.

I'm not sure whether it actually matter because i was not able
to come up with a testcase, but it looks a bit cleaner anyway..

spatel added inline comments.Apr 18 2018, 10:41 AM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2495–2497 ↗(On Diff #142612)

Make sure I haven't missed anything, but it seems easier to match this inline here more simply as:

// Eliminate a 'not' from xor form of masked-merge (aka, bitwise select).
// The 'not' must be operand 1 of the 'and' (complexity-based op ordering).
// ((Op1 ^ X) & ~Y) ^ Op1 --> ((Op1 ^ X) & Y) ^ X
if (match(Op0, m_OneUse(m_And(m_c_Xor(m_Specific(Op1), m_Value(X)),
                              m_Not(m_Value(Y)))))) {
  Value *Op00 = cast<BinaryOperator>(Op0)->getOperand(0);
  return BinaryOperator::CreateXor(Builder.CreateAnd(Op00, Y), X);
}
// Op0 ^ ((Op0 ^ X) & ~Y) --> ((Op0 ^ X) & Y) ^ X
if (match(Op1, m_OneUse(m_And(m_c_Xor(m_Specific(Op0), m_Value(X)),
                              m_Not(m_Value(Y)))))) {
  Value *Op10 = cast<BinaryOperator>(Op1)->getOperand(0);
  return BinaryOperator::CreateXor(Builder.CreateAnd(Op10, Y), X);
}
lebedev.ri added inline comments.Apr 19 2018, 9:32 AM
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2495–2497 ↗(On Diff #142612)

I'm sorry, i don't really like this, especially after seeing the DAGCombiner :)
I'm going to work on fixing the commutability problem of BinaryOp_match and m_Specific().
(if that fix won't be accepted, will revisit this.)

Streamlined the matcher, rebased ontop of revisited multiuse tests.

  • Rebased.
  • Ping. This differential is the base, and has no unmerged dependencies, so this is the one to review :)
spatel accepted this revision.Apr 28 2018, 7:50 AM

LGTM - see inline for some nits.

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2414–2420 ↗(On Diff #144443)

The comment seems inverted from our usual description of:
Starting pattern --> Ending pattern

2432 ↗(On Diff #144443)

Call this 'NotM' to comply with LLVM capitalization rules?

This revision is now accepted and ready to land.Apr 28 2018, 7:50 AM
lebedev.ri marked 3 inline comments as done.Apr 28 2018, 8:48 AM

@spatel thank you for the review!

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2414–2420 ↗(On Diff #144443)

It is because this very function will be dealing with unfolding the pattern in case of constant mask (D45867), then the description will be appended.
But i tried to reword it a little.

This revision was automatically updated to reflect the committed changes.