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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

The uploaded diff is against the previous revision of this patch. Please rebase so it is against (and can be applied cleanly to) the main branch.



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


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2103
+
+define i1 @samesign(i32 %x, i32 %y) {
+; CHECK-LABEL: @samesign(
----------------
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
}
```


================
Comment at: llvm/test/Transforms/InstCombine/and-or-icmps.ll:2156
+
+define i1 @samesign_inverted(i32 %x, i32 %y) {
+  ; CHECK-LABEL: @samesign_inverted(
----------------
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
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127903