[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:2643
     if (((TrueIfSignedL && !TrueIfSignedR &&
-          match(LHS0, m_And(m_Value(LHSX), m_Value(LHSY))) &&
-          match(RHS0, m_Or(m_Value(RHSX), m_Value(RHSY)))) ||
+          (IsAnd ? match(LHS0, m_c_Or(m_Value(X), m_Value(Y)))
+                 : match(LHS0, m_c_And(m_Value(X), m_Value(Y)))) &&
----------------
spatel wrote:
> It doesn't make sense to use the commutative matcher on the first match - we're matching 2 arbitrary values, so it will succeed either way.
Good call, that makes sense - will amend in updated diff (which will be against main, not stacked on previous diff).


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2103
+
+define i1 @samesign(i32 %x, i32 %y) {
+; CHECK-LABEL: @samesign(
----------------
spatel wrote:
> We should have tests for commuted patterns like:
> 
> ```
> define i1 @samesign_commute1(i32 %x, i32 %y) {
> 	%a = and i32 %x, %y
> 	%lt = icmp slt i32 %a, 0
> 	%o = or i32 %x, %y
> 	%gt = icmp sgt i32 %o, -1
> 	%r = or i1 %gt, %lt  ; compares are swapped
> 	ret i1 %r
> }
> 
> define i1 @samesign_commute2(i32 %x, i32 %y) {
> 	%a = and i32 %x, %y
> 	%lt = icmp slt i32 %a, 0
> 	%o = or i32 %y, %x  ; source values are commuted
> 	%gt = icmp sgt i32 %o, -1
> 	%r = or i1 %lt, %gt
> 	ret i1 %r
> }
> 
> define i1 @samesign_commute3(i32 %x, i32 %y) {
> 	%a = and i32 %x, %y
> 	%lt = icmp slt i32 %a, 0
> 	%o = or i32 %y, %x  ; source values are commuted
> 	%gt = icmp sgt i32 %o, -1
> 	%r = or i1 %gt, %lt ; compares are swapped
> 	ret i1 %r
> }
> ```
Oh yes I see - cool thank you I will get these commuted tests included in next diff.


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2156
+
+define i1 @samesign_inverted(i32 %x, i32 %y) {
+  ; CHECK-LABEL: @samesign_inverted(
----------------
spatel wrote:
> We should have "negative" tests for patterns that fail to match one or more of the constraints. Example:
> 
> ```
> define i1 @samesign_inverted_wrong_cmp(i32 %x, i32 %y) {
>   %a = and i32 %x, %y
>   %gt = icmp sgt i32 %a, 0 ; this is not a sign-bit test
>   %o = or i32 %x, %y
>   %lt = icmp slt i32 %o, 0
>   %r = and i1 %gt, %lt
>   ret i1 %r
> }
> ```
Ah I see, gotcha - thanks for your help I really appreciate it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127903