<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Oct 18, 2014 at 9:37 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><div>----- Original Message -----<br>
> From: "David Majnemer" <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>><br>
> To: "ankur29 garg" <<a href="mailto:ankur29.garg@samsung.com" target="_blank">ankur29.garg@samsung.com</a>>, <a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>, "suyog sarda" <<a href="mailto:suyog.sarda@samsung.com" target="_blank">suyog.sarda@samsung.com</a>>, "david<br>
> majnemer" <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>><br>
> Cc: <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> Sent: Saturday, October 18, 2014 4:47:12 AM<br>
> Subject: Re: [PATCH] [InstCombineAddSub] Added the transformation - "sub (or A B) (xor A B) -> (and A B)"<br>
><br>
> InstCombine is not supposed to handle every possible just because 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 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>
</div></div>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 superoptimizer), it is really hard to predict what patterns will occur in practice -- and given knowledge of certain bits from switch statements, enums (range metadata), etc. combined with LLVM's canonicalization in terms of large integers, and several other factors, lead to many patterns that I would consider strange, and not-uncommonly in performance-sensitive regions of code. </blockquote><div><br></div><div>Sure, it is very hard to predict what is and is not useful. But say, self hosting LLVM, building a large project like Chrome, an OS kernel or some benchmark like Spec would tell us what would happen with a particular corpus. If we never know that the transform fires, why have it?</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">
<br>
- We (obviously) should not engage in premature optimization -- I've seen no evidence that matching these kinds of patterns in InstCombine contributes a significant portion of its running time. And until such evidence is presented, we should not make statements about expense (there is no complexity argument here). In any case, looking at combinations of three binary operators, are there any more than a few hundred that could simplify in any reasonable sense? Probably not even that many. I see no reason we should not handle them all. </blockquote><div><br></div><div>I view this sort of change as a hypothetical improvement which is more or less benign, it doesn't really hurt or help InstCombine. Perhaps I'm just afraid of "death by a thousand cuts", maybe *I* am engaging in premature optimization by thinking along these lines.</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">
<br>
- For simplifications that require certain bits to be known, calls into ValueTracking can be expensive, but making duplicate calls into ValueTracking is a separate issue that can be fixed if it becomes important.<br>
<br>
- I feel that, in the past, the real engineering tradeoff was slightly different: it used to be difficult to prove the transformations correct. As a result, there was a significant possibility of bugs being introduced by each of these transformations, and it made more sense to limit them. As you know, this is no longer anywhere near as true as it was: we now have good tools to verify these transformations.<br></blockquote><div><br></div><div>It's still true though. Verifying that a single combine doesn't tell us if the combine is safe. I have personally fixed combines which resulted in infinite loops because the transform would conflict with some other transform.</div><div>Both transforms, when formulated for Z3/Alive/whatever, would be "correct" but are obviously wrong in the greater context of InstCombine. Every change carries some risk, this change is very low risk but it provides no known benefit.<br></div><div><br></div><div>Some of my concern lies with adding transforms which make the canonicalization of IR go one way or the another when in practice we don't actually care because the IR is not known to ever occur in the real world.<br></div><div>However, we now have to keep yet another transform in mind when determining if a new transform is sound.</div><div> </div><div>In the greater scheme of things, Rome won't fall if we add this transform. In fact, I think it's more reasonable than not to have this particular transform. I just don't think the bar used to judge a new InstCombine should be so low as to roll around on the ground.</div><div><br></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">
<br>
Thanks again,<br>
Hal<br>
<div><div><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" target="_blank">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>
</div></div><span><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>