This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Use commutable and/or/xor matchers to simplify some code
ClosedPublic

Authored by craig.topper on Apr 4 2017, 3:02 PM.

Details

Summary

This is my first time using the commutable matchers so wanted to make sure I was doing it right.

Are there any other matcher tricks to further shrink this? Can we commute the whole match so we don't have to LHS and RHS separately?

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 4 2017, 3:02 PM
spatel edited edge metadata.EditedApr 4 2017, 4:18 PM

This is the minimum improvement, but we can do better. :)

  1. It's unlikely that we actually have tests for all of the commuted variants. I noticed that when I made a change to the operand complexity values a few months ago. Adding the missing cases would be a nice improvement.
  2. The nullptr initialization for the captured Values isn't necessary.
  3. We don't have an m_c_Add, but we probably should for situations like these.
  4. Even if we had an m_c_Add, I don't think we'd be able to use it with m_Specific in this case because we need the generic half to capture first. I haven't actually tested that, so it's worth an experiment.
  5. If #4 is not an option, then we can do one of these (and there's no consensus in existing code about which is best):
    1. Glom the match statements together with a big '||'. (At least get rid of the duplicated code within each 'if' clause)
    2. Test if the xor is on the right and std::swap the add operands.
    3. Use normal value capture and check for the operand equality after that:
Value *A, *B, *C, *D;
if (match(&I, m_c_Add(m_Xor(m_Value(A), m_Value(B)),
                      m_And(m_Value(C), m_Value(D)))) &&
    ((A == C && B == D) || (A == D && B == C)))
  return BinaryOperator::CreateOr(A, B);
davide accepted this revision.Apr 9 2017, 3:30 PM

I think this is a decent cleanup per-se, makes the code more readable, so it can go in. I agree with Sanjay it can be improved, but that can be left as a follow-up.

This revision is now accepted and ready to land.Apr 9 2017, 3:30 PM
This revision was automatically updated to reflect the committed changes.