[PATCH] D127903: [InstCombine] Optimize test for same-sign of values

[Eric Gullufsen via Phabricator via llvm-commits]

emgullufsen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2638
+  bool TrueIfSignedL, TrueIfSignedR;
+  if (!IsAnd && InstCombiner::isSignBitCheck(PredL, *LHSC, TrueIfSignedL) &&
+      InstCombiner::isSignBitCheck(PredR, *RHSC, TrueIfSignedR)) {
----------------
spatel wrote:
> It's fine to ignore the inverted logic pattern that ends with an 'and' instruction, but please put a TODO comment on this, so we know it can be handled in a follow-up patch.
Ah good call - will amend in updated patch (and will start looking at the inverted pattern (for a separate patch).


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2643
+          match(LHS0, m_And(m_Value(LHSX), m_Value(LHSY))) &&
+          match(RHS0, m_Or(m_Value(RHSX), m_Value(RHSY)))) ||
+         (!TrueIfSignedL && TrueIfSignedR &&
----------------
spatel wrote:
> Have a look at the `m_Specific` matcher - we really only have 2 variables in this pattern, so you can just call them X and Y as shown in your code comment.
> 
> To match commuted patterns, see `m_c_Or` / `m_c_And`. 
> 
> Check this file for examples of existing code that uses those calls.
ack - I ought to have thought to use the commutative m_c_ matchers - thank you, will amend in updated patch


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2651
+      APInt W = APInt(32, -1, true);
+      Value *NewC = ConstantInt::get(NewXor->getType(), W);
+      return Builder.CreateICmp(ICmpInst::ICMP_SGT, NewXor, NewC);
----------------
spatel wrote:
> ConstantInt::getAllOnesValue()
very cool thank you I knew there had to be a better way for that construction - will amend in updated patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127903/new/

https://reviews.llvm.org/D127903