<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Oct 18, 2014 at 9:51 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">----- Original Message -----<br>
> From: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> To: <a href="mailto:reviews%2BD5719%2Bpublic%2B3088893b38cafcf5@reviews.llvm.org">reviews+D5719+public+3088893b38cafcf5@reviews.llvm.org</a><br>
> Cc: "ankur29 garg" <<a href="mailto:ankur29.garg@samsung.com">ankur29.garg@samsung.com</a>>, "suyog sarda" <<a href="mailto:suyog.sarda@samsung.com">suyog.sarda@samsung.com</a>>, <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Saturday, October 18, 2014 11:37:40 PM<br>
> Subject: Re: [PATCH] [InstCombineAddSub] Added the transformation - "sub (or  A       B) (xor A B) -> (and A B)"<br>
><br>
> ----- Original Message -----<br>
> > From: "David Majnemer" <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>><br>
> > To: "ankur29 garg" <<a href="mailto:ankur29.garg@samsung.com">ankur29.garg@samsung.com</a>>,<br>
> > <a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>, "suyog sarda" <<a href="mailto:suyog.sarda@samsung.com">suyog.sarda@samsung.com</a>>,<br>
> > "david<br>
> > majnemer" <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>><br>
> > Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > Sent: Saturday, October 18, 2014 4:47:12 AM<br>
> > Subject: Re: [PATCH] [InstCombineAddSub] Added the transformation -<br>
> > "sub (or A  B) (xor A B) -> (and A B)"<br>
> ><br>
> > InstCombine is not supposed to handle every possible just because<br>
> > it<br>
> > is theoretically possible case.  It's engineered to be a series of<br>
> > engineering tradeoffs.  I don't know what the history of the<br>
> > similar<br>
> > transform is in VisitAdd, it's possible that it wasn't added in a<br>
> > principled way.<br>
> ><br>
> > If we were to arbitrarily add patterns, we would eventually make<br>
> > InstCombine cripplingly slow.<br>
><br>
> To be honest, I think this position is unhelpful. A few thoughts:<br>
><br>
>  - In my experience (derived from my work on an instruction-level<br>
>  superoptimizer), it is really hard to predict what patterns will<br>
>  occur in practice -- and given knowledge of certain bits from<br>
>  switch statements, enums (range metadata), etc. combined with<br>
>  LLVM's canonicalization in terms of large integers, and several<br>
>  other factors, lead to many patterns that I would consider strange,<br>
>  and not-uncommonly in performance-sensitive regions of code.<br>
<br>
</div></div>To add another anecdote, just when I thought I had classified the kinds of patterns that were occurring in practice, I started looking at instrumented code (address sanitizer, etc.), and I learned that I had a lot left to do.<br></blockquote><div><br></div><div>I think that makes an excellent argument for looking at what happens in practice and focusing optimizations to improve those circumstances.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888"><br>
 -Hal<br>
</font></span><div class=""><div class="h5"><br>
><br>
>  - We (obviously) should not engage in premature optimization -- I've<br>
>  seen no evidence that matching these kinds of patterns in<br>
>  InstCombine contributes a significant portion of its running time.<br>
>  And until such evidence is presented, we should not make statements<br>
>  about expense (there is no complexity argument here). In any case,<br>
>  looking at combinations of three binary operators, are there any<br>
>  more than a few hundred that could simplify in any reasonable<br>
>  sense? Probably not even that many. I see no reason we should not<br>
>  handle them all.<br>
><br>
>  - For simplifications that require certain bits to be known, calls<br>
>  into ValueTracking can be expensive, but making duplicate calls<br>
>  into ValueTracking is a separate issue that can be fixed if it<br>
>  becomes important.<br>
><br>
>  - I feel that, in the past, the real engineering tradeoff was<br>
>  slightly different: it used to be difficult to prove the<br>
>  transformations correct. As a result, there was a significant<br>
>  possibility of bugs being introduced by each of these<br>
>  transformations, and it made more sense to limit them. As you know,<br>
>  this is no longer anywhere near as true as it was: we now have good<br>
>  tools to verify these transformations.<br>
><br>
> Thanks again,<br>
> Hal<br>
><br>
> ><br>
> > <a href="http://reviews.llvm.org/D5719" target="_blank">http://reviews.llvm.org/D5719</a><br>
> ><br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>