<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 11/25/2014 04:33 AM, Erik Eckstein
wrote:<br>
</div>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">Hi,
this is a patch for InstCombine. It tries to simplify
(icmp sgt x, -1) & (icmp sgt/sge n, x)
to
icmp ugt/uge n, x
in case n is not negative.</pre>
</div>
</blockquote>
I'm generally happy with this change, but have a few minor comments
inline. In the future, could you a) use phabricator and b) include
more context in the diffs?<br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
</pre>
</div>
<br>
<fieldset class="mimeAttachmentHeader"><legend
class="mimeAttachmentHeaderName">llvm-simplify-range-check.patch</legend></fieldset>
<br>
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">Index: lib/Transforms/InstCombine/InstCombine.h
===================================================================
--- lib/Transforms/InstCombine/InstCombine.h (revision 222749)
+++ lib/Transforms/InstCombine/InstCombine.h (working copy)
@@ -162,6 +162,7 @@
Instruction *visitUDiv(BinaryOperator &I);
Instruction *visitSDiv(BinaryOperator &I);
Instruction *visitFDiv(BinaryOperator &I);
+ Value *simplifyRangeCheck(ICmpInst *Op0, ICmpInst *Op1);
Value *FoldAndOfICmps(ICmpInst *LHS, ICmpInst *RHS);
Value *FoldAndOfFCmps(FCmpInst *LHS, FCmpInst *RHS);
Instruction *visitAnd(BinaryOperator &I);
Index: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (revision 222749)
+++ lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (working copy)
@@ -785,6 +785,43 @@
return nullptr;
}
+/// Try to fold (icmp sgt x, -1) & (icmp sgt/sge n, x) --> icmp ugt/uge n, x</pre>
</div>
</blockquote>
Is there a reason you're only handling the upper bound phrased as a
sgt/sge? Do we canonicalize away the alternate slt/sle formation?<br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
+Value *InstCombiner::simplifyRangeCheck(ICmpInst *Cmp0, ICmpInst *Cmp1) {</pre>
</div>
</blockquote>
This doesn't need to be a member function does it? A static
function would be preferred.<br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
+ // Check the lower range comparison: x >= 0
+ ConstantInt *RangeStart = dyn_cast<ConstantInt>(Cmp0->getOperand(1));
+ if (!RangeStart)
+ return nullptr;
+
+ if (!RangeStart->isMinusOne() || Cmp0->getPredicate() != ICmpInst::ICMP_SGT)
+ return nullptr;</pre>
</div>
</blockquote>
It seems like you're relying on an existing canonicalization to
signed x > -1? If so, add that to your comment. <br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
+
+ // Only do the optimization if the compares are not used otherwise.
+ if (!Cmp0->hasOneUse() || !Cmp1->hasOneUse())
+ return nullptr;</pre>
</div>
</blockquote>
You should remove this check and create a new instruction. <br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
+
+ // Check it both icmp compare against the same value.
+ if (Cmp1->getOperand(1) != Cmp0->getOperand(0))
+ return nullptr;</pre>
</div>
</blockquote>
It might be clearer to use named temporises here:<br>
Value* X1 = Cmp0->getOperand(0);<br>
Value* X2 = Cmp1->getOperand(1)<br>
Value* N = Cmp1->getOperand(0);<br>
if (X1 != X2) return nullptr;<br>
...etc..<br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
+
+ // Check the upper range comparison.
+ ICmpInst::Predicate NewPred;
+ switch (Cmp1->getPredicate()) {
+ case ICmpInst::ICMP_SGT: NewPred = ICmpInst::ICMP_UGT; break;
+ case ICmpInst::ICMP_SGE: NewPred = ICmpInst::ICMP_UGE; break;
+ default: return nullptr;
+ }
+
+ // This simplification is only valid if the upper range is not negative.
+ bool IsNegative, IsNotNegative;
+ ComputeSignBit(Cmp1->getOperand(0), IsNotNegative, IsNegative, DL, 0, AT,
+ Cmp1, DT);
+ if (!IsNotNegative)
+ return nullptr;
+
+ Cmp1->setPredicate(NewPred);</pre>
</div>
</blockquote>
Please create a new icmp rather than modifying one in place. <br>
<blockquote
cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com"
type="cite">
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
+ return Cmp1;
+}
+
/// FoldAndOfICmps - Fold (icmp)&(icmp) if possible.
Value *InstCombiner::FoldAndOfICmps(ICmpInst *LHS, ICmpInst *RHS) {
ICmpInst::Predicate LHSCC = LHS->getPredicate(), RHSCC = RHS->getPredicate();
@@ -807,6 +844,14 @@
if (Value *V = foldLogOpOfMaskedICmps(LHS, RHS, true, Builder))
return V;
+ /// (icmp sgt x, -1) & (icmp sgt/sge n, x) --> icmp ugt/uge n, x
+ if (Value *V = simplifyRangeCheck(LHS, RHS))
+ return V;
+
+ /// (icmp sgt/sge n, x) & (icmp sgt x, -1) --> icmp ugt/uge n, x
+ if (Value *V = simplifyRangeCheck(RHS, LHS))
+ return V;
+
// This only handles icmp of constants: (icmp1 A, C1) & (icmp2 B, C2).
Value *Val = LHS->getOperand(0), *Val2 = RHS->getOperand(0);
ConstantInt *LHSCst = dyn_cast<ConstantInt>(LHS->getOperand(1));
Index: test/Transforms/InstCombine/range-check.ll
===================================================================
--- test/Transforms/InstCombine/range-check.ll (revision 0)
+++ test/Transforms/InstCombine/range-check.ll (working copy)
@@ -0,0 +1,117 @@
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+; Check simplification of
+; (icmp sgt x, -1) & (icmp sgt/sge n, x) --> icmp ugt/uge n, x
+
+; CHECK-LABEL: define i1 @test1
+; CHECK: %b = icmp ugt i32 %nn, %x
+; CHECK: ret i1 %b
+define i1 @test1(i32 %x, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sge i32 %x, 0
+ %b = icmp slt i32 %x, %nn
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @test2
+; CHECK: %b = icmp uge i32 %nn, %x
+; CHECK: ret i1 %b
+define i1 @test2(i32 %x, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sgt i32 %x, -1
+ %b = icmp sle i32 %x, %nn
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @test3
+; CHECK: %a = icmp ugt i32 %nn, %x
+; CHECK: ret i1 %a
+define i1 @test3(i32 %x, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sgt i32 %nn, %x
+ %b = icmp sge i32 %x, 0
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @test4
+; CHECK: %a = icmp uge i32 %nn, %x
+; CHECK: ret i1 %a
+define i1 @test4(i32 %x, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sge i32 %nn, %x
+ %b = icmp sge i32 %x, 0
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; Negative tests
+
+; CHECK-LABEL: define i1 @negative1
+; CHECK: %a = icmp
+; CHECK: %b = icmp
+; CHECK: %c = and i1 %a, %b
+; CHECK: ret i1 %c
+define i1 @negative1(i32 %x, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sgt i32 %nn, %x
+ %b = icmp sgt i32 %x, 0
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @negative2
+; CHECK: %a = icmp
+; CHECK: %b = icmp
+; CHECK: %c = and i1 %a, %b
+; CHECK: ret i1 %c
+define i1 @negative2(i32 %x, i32 %n) {
+ %a = icmp sgt i32 %n, %x
+ %b = icmp sge i32 %x, 0
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @negative3
+; CHECK: %a = icmp
+; CHECK: %b = icmp
+; CHECK: %c = and i1 %a, %b
+; CHECK: ret i1 %c
+define i1 @negative3(i32 %x, i32 %y, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sgt i32 %nn, %x
+ %b = icmp sge i32 %y, 0
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @negative4
+; CHECK: %a = icmp
+; CHECK: %b = icmp
+; CHECK: %c = and i1 %a, %b
+; CHECK: ret i1 %c
+define i1 @negative4(i32 %x, i32 %n) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp ne i32 %nn, %x
+ %b = icmp sge i32 %x, 0
+ %c = and i1 %a, %b
+ ret i1 %c
+}
+
+; CHECK-LABEL: define i1 @negative5
+; CHECK: %a = icmp
+; CHECK: %b = icmp
+; CHECK: %c = and i1 %a, %b
+; CHECK: ret i1 %c
+define i1 @negative5(i32 %x, i32 %n, i32 * %p) {
+ %nn = and i32 %n, 2147483647
+ %a = icmp sgt i32 %nn, %x
+ %b = icmp sge i32 %x, 0
+ %c = and i1 %a, %b
+ %ae = sext i1 %a to i32
+ store i32 %ae, i32 * %p
+ ret i1 %c
+}
+
</pre>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<div class="moz-text-plain" wrap="true" graphical-quote="true"
style="font-family: -moz-fixed; font-size: 12px;"
lang="x-western">
<pre wrap="">
Erik
</pre>
</div>
</blockquote>
<br>
</body>
</html>