<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Thanks for the detailed comments!<div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div bgcolor="#FFFFFF" text="#000000" class="">In the future, could you a) use phabricator and b) include more context in the diffs?</div></blockquote><br class=""></div><div class="">Done -> <a href="http://reviews.llvm.org/D6422" class="">http://reviews.llvm.org/D6422</a></div><div class="">I added my comments to this email there.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Erik</div><div class=""><br class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 26 Nov 2014, at 02:54, Philip Reames <<a href="mailto:listmail@philipreames.com" class="">listmail@philipreames.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<br class="">
<div class="moz-cite-prefix">On 11/25/2014 04:33 AM, Erik Eckstein
wrote:<br class="">
</div>
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">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 class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">
</pre>
</div>
<br class="">
<fieldset class="mimeAttachmentHeader"><legend class="mimeAttachmentHeaderName">llvm-simplify-range-check.patch</legend></fieldset>
<br class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">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 class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">+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 class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">+ // 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 class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">+
+ // 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 class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">+
+ // 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 class="">
Value* X1 = Cmp0->getOperand(0);<br class="">
Value* X2 = Cmp1->getOperand(1)<br class="">
Value* N = Cmp1->getOperand(0);<br class="">
if (X1 != X2) return nullptr;<br class="">
...etc..<br class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">+
+ // 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 class="">
<blockquote cite="mid:8911B10E-3F4B-4F87-AFD9-EE5047EA7CDE@apple.com" type="cite" class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">+ 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 class="">
<fieldset class="mimeAttachmentHeader"></fieldset>
<br class="">
<div class="moz-text-plain" wrap="true" graphical-quote="true" style="font-family: -moz-fixed; font-size: 12px;" lang="x-western">
<pre wrap="" class="">Erik
</pre>
</div>
</blockquote>
<br class="">
</div>
</div></blockquote></div><br class=""></div></body></html>