<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 19, 2017 at 5:27 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_6591352667863351406gmail-m_-436234310287858029gmail-HOEnZb"><div class="m_6591352667863351406gmail-m_-436234310287858029gmail-h5">On Fri, May 19, 2017 at 4:11 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:<br>
> [adding llvm-dev for wider audience]<br>
><br>
> On Fri, May 19, 2017 at 12:28 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Fri, May 19, 2017 at 11:00 AM, Davide Italiano <<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>><br>
>> wrote:<br>
>>><br>
>>> On Fri, May 19, 2017 at 10:00 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>><br>
>>> wrote:<br>
>>> > Is "VRP" (value range propagation) in LLVM-speak "CVP" (correlated<br>
>>> > value<br>
>>> > propagation)?<br>
>>> ><br>
>>> > If so, we have this comment regarding compares:<br>
>>> > // As a policy choice, we choose not to waste compile time on<br>
>>> > anything<br>
>>> > where<br>
>>> > // the comparison is testing local values.<br>
>>> ><br>
>>> > Or this for div/rem:<br>
>>> > // As a policy choice, we choose not<br>
>>> > // to waste compile time on anything where the operands are local<br>
>>> > defs.<br>
>>> ><br>
>>> > "Local" means in the same basic block from what I can tell by the code<br>
>>> > here.<br>
>>> ><br>
>>> > I think this means that this pass explicitly defers handling simple<br>
>>> > cases<br>
>>> > like:<br>
>>> > <a href="https://reviews.llvm.org/D33342" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3334<wbr>2</a><br>
>>> > ...to another pass, and currently that pass is InstCombine (because the<br>
>>> > patterns really can't be any simpler than the tests in that patch,<br>
>>> > right?).<br>
>>> ><br>
>>> > I think that improving compile-time should not preclude improving<br>
>>> > InstCombine. We should do both.<br>
>>> ><br>
>>><br>
>>> Just thoughts, feel free to ignore them.<br>
>>> I didn't measure the impact in LLVM, but I'm sure you can do VRP<br>
>>> relatively fast (GCC does that both interprocedurally and<br>
>>> intraprocedurally and their pass is much faster in some cases than<br>
>>> LLVM's), i.e. O(N) in practice, so, maybe we could re-evaluate this<br>
>>> policy?<br>
>><br>
>><br>
>> Yeah, that's kind of silly, we can do much better.<br>
>><br>
>>><br>
>>> I think replacing a pass in LLVM is not trivial (I'm learning it the<br>
>>> hard way with NewGVN). OTOH, I'm still not entirely convinced<br>
>>> `InstCombine` should handle these cases given it's already a<br>
>>> compile-time sink?<br>
>><br>
>><br>
><br>
> Correct me where I'm going wrong.<br>
><br>
> 1. We got a bug report with a perf regression that was root caused to this<br>
> IR:<br>
><br>
> define i1 @xor_of_icmps(i64 %a) {<br>
> %b = icmp sgt i64 %a, 0<br>
> %c = icmp eq i64 %a, 1<br>
> %r = xor i1 %b, %c<br>
> ret i1 %r<br>
> }<br>
><br>
> Because this was a regression, we know that the optimal/canonical IR for<br>
> this program is:<br>
><br>
> define i1 @xor_of_icmps_canonical(i64 %a) {<br>
> %r = icmp sgt i64 %a, 1<br>
> ret i1 %r<br>
> }<br>
><br>
> 2. I saw this as a bug in InstCombine because I think InstCombine's job is<br>
> to canonicalize simple IR sequences.<br>
><br>
> 3. I assumed that matching an (xor icmp, icmp) pattern can be done<br>
> efficiently in InstCombine. In fact, I knew that we already did that match,<br>
> so there is zero incremental cost to find this pattern in InstCombine.<br>
><br>
> 4. I knew that we already handle related and-of-cmps and or-of-cmps patterns<br>
> in InstSimplify/InstCombine.<br>
><br>
> 5. Based on that, I have proposed a patch that mostly uses existing logic in<br>
> those passes to solve this bug in the most general way I am aware of. It<br>
> makes 2 calls to InstSimplify to find a potential fold before<br>
> morphing/creating new instructions that may get folded by other existing<br>
> logic.<br>
><br>
><br>
> Questions:<br>
> 1. Was/is InstCombine intended to handle this transform?<br>
><br>
> 2. If there is a measurable compile-time cost for this patch, then there<br>
> must be a structural problem with InstSimplify, InstCombine, and/or the pass<br>
> pipeline?<br>
><br>
> 3. Is there another pass that is more suitable to solve this bug than<br>
> InstCombine? CVP was mentioned as a candidate for enhancement. Any others?<br>
><br>
> 4. If there is a more suitable pass, why is it more suitable?<br>
><br>
<br>
</div></div>I can provide an example + analysis: <a href="https://reviews.llvm.org/D33172#754563" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3317<wbr>2#754563</a><br>
<span class="m_6591352667863351406gmail-m_-436234310287858029gmail-"><br></span></blockquote><div><br></div><div>Yes, I agreed that case should be solved more generally. There are follow-up questions in that review that I'd like to reach consensus on though. For example:<br><br>define i32 @cmp_br_1(i32 %a, i32 %b) {<br>entry:<br> %cmp = icmp sge i32 %a, %b<br> br i1 %cmp, label %t, label %f<br>t:<br> ret i32 42<br>f:<br> ret i32 31415<br>}<br><br>define i32 @cmp_br_2(i32 %a, i32 %b) {<br>entry:<br> %cmp = icmp slt i32 %a, %b ; invert predicate<br> br i1 %cmp, label %f, label %t ; swap successors<br><br>t: <br> ret i32 42<br><br>f:<br> ret i32 31415<br>}<br><br></div><div>Which of these is canonical? Is InstCombine responsible for that canonicalization?<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_6591352667863351406gmail-m_-436234310287858029gmail-">
> 5. If we add to that pass or create a new pass that specifically targets<br>
> logic-of-cmps, can we pull that code out of InstSimplify and InstCombine?<br>
><br>
> 6. If compile-time has become unbearable, shouldn't we be moving<br>
> optimization logic and/or passes from -O1 to -O2 to -O3, etc? My<br>
> understanding as a compiler consumer has always been that I'll get better<br>
> performing code the higher up the -O# ladder I climb, but it will cost<br>
> increasingly more time to compile. Is that not the model for LLVM?<br>
><br>
<br>
</span>The real problem of instcombine IMHO is that it suffer from an<br>
additive effect. i.e. matchers over matchers are added over time which<br>
per-se don't slow down the compiler in any measurable manner. But all<br>
of them sum up result in a significant compiler slowdown, and we<br>
notice only when it's too late (i.e. when the code has been released).<br>
We've all under the assumption that the time in InstCombine was spent<br>
inside auxiliary analysis computing the bitwise domain<br>
(ComputeKnownBits et similia). Turns out we're not able to find a case<br>
where caching speeds up things.<br>
<span class="m_6591352667863351406gmail-m_-436234310287858029gmail-"><br></span></blockquote><div><br></div><div>This conclusion doesn't line up with the experimental results I showed here:<br><a href="http://lists.llvm.org/pipermail/llvm-dev/2017-March/111416.html" target="_blank">http://lists.llvm.org/<wbr>pipermail/llvm-dev/2017-March/<wbr>111416.html</a><br><br></div><div>Is there a bug report with a test case for the stated case of "matchers over matchers"? I'd like to try some experiments with that test. From the earlier thread, it sounded like improving that case was something that was being investigated or would be investigated soon.<br><br></div><div>The general idea, as I understand it, is that we need to better define/distinguish canonicalization from optimization and then separate InstCombine into 'InstCanonicalize' and 'InstOptimize' and/or not run InstCombine at full strength so many times.<br></div><div><br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_6591352667863351406gmail-m_-436234310287858029gmail-">
><br>
>> People seem intent on adding lots and lots of stuff to instcombine because<br>
>> it's easy.<br>
>> This makes it harder to ever replace, while simultaneously making it<br>
>> slower.<br>
>> It's not hard to see where that path ends up.<br>
>> InstCombine does a lot of random crap that isn't even combining or graph<br>
>> rewrite, too, because well second and third order effects are hard.<br>
><br>
><br>
> Can you provide some examples? I honestly don't know where I should draw the<br>
> line. If I've crossed the line, I'd like to fix it.<br>
><br>
<br>
</span>I don't have the code handy, but something that came up recently (and<br>
I was a little surprised) is that instcombine checks for irreducible<br>
PHI cycles, see the functions/comments in InstCombinePHI.<br>
<span class="m_6591352667863351406gmail-m_-436234310287858029gmail-im m_6591352667863351406gmail-m_-436234310287858029gmail-HOEnZb"><br>
>> If we want an optimal graph rewriter, we should actually just build one,<br>
>> with sane rewrite rules, verification that it fixpoints, and some sense of<br>
>> good ordering, etc, instead of slowly every adding possible pattern match<br>
>> to InstCombine.<br>
><br>
><br>
> Can you recommend papers or projects to look at to get a better<br>
> understanding of an optimal graph rewriter? LLVM is the only compiler I've<br>
> worked on from the inside, so I don't really know what else is possible /<br>
> feasible.<br>
><br>
><br>
<br>
<br>
<br>
</span><div class="m_6591352667863351406gmail-m_-436234310287858029gmail-HOEnZb"><div class="m_6591352667863351406gmail-m_-436234310287858029gmail-h5">--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</div></div></blockquote></div><br></div></div>