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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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)) {
----------------
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.


================
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 &&
----------------
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.


================
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);
----------------
ConstantInt::getAllOnesValue()


================
Comment at: llvm/test/Transforms/InstCombine/same-sign-naive.ll:3
+
+define i1 @samesign(i32 %x, i32 %y) {
+; CHECK-LABEL: @samesign(
----------------
You can tack more tests on to the end of related tests in "and-or-icmps.ll". 
You can also get an idea about how to vary tests to get better coverage by looking at existing tests.
One common variation to test (and code for): what happens if the intermediate values have other uses?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127903