<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 25, 2013 at 10:16 AM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank" class="cremed">stoklund@2pi.dk</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="im"><br>
On Jan 24, 2013, at 4:39 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" class="cremed">chandlerc@gmail.com</a>> wrote:<br>
<br>
> Author: chandlerc<br>
> Date: Thu Jan 24 06:39:29 2013<br>
> New Revision: 173342<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=173342&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=173342&view=rev</a><br>
> Log:<br>
> Plug TTI into the speculation logic, giving it a real cost interface<br>
> that can be specialized by targets.<br>
><br>
> The goal here is not to be more aggressive, but to just be more accurate<br>
> with very obvious cases. There are instructions which are known to be<br>
> truly free and which were not being modeled as such in this code -- see<br>
> the regression test which is distilled from an inner loop of zlib.<br>
<br>
</div>Hi Chandler,<br>
<br>
It is important to realize how profoundly clueless the IR passes are about execution speed. The obvious cases you mention here could easily cause regressions. (Just like the existing heuristics can).<br></blockquote><div>
<br></div><div style>I think I'm relatively aware here, and in fact really do agree.</div><div style><br></div><div style>But this patch isn't substantively changing what's going on here. We *already* had a completely broken and terrible cost model, and did brain-dead if-conversion on it. This patch makes the if-conversions performed at the IR level somewhat more *consistent*. That was the goal.</div>
<div style><br></div><div style>The concrete code that motivated me to get here was code that would oscillate between being if-converted here and not being if-converted here because of "no-op" transforms elsewhere in the IR optimizer:</div>
<div style><br></div><div style>--------</div><div style>  %cmp = icmp ... i16 %a, %b</div><div style>  br i1 %cmp, label %true, label %false</div><div style><br></div><div style>true:</div><div style>  %sub = add i16 %a, %b</div>
<div style>  br label %false</div><div style><br></div><div style>false:</div><div style>  %phi = i16 [ %a, ... ], [ %sub, %true ]</div><div style>--------</div><div style>became:</div><div style><div>--------</div><div style>
  %a.conv = zext i16 %a to i32</div><div style>  %b.conv = zext i16 %b to i32</div><div>  %cmp = icmp ... i32 %a.conv, %b.conv</div><div>  br i1 %cmp, label %true, label %false</div><div><br></div><div>true:</div><div>  %sub = add i32 %a.conv, %b.conv</div>
<div style>  %sub.conv = trunc i32 %sub to i16</div><div>  br label %false</div><div><br></div><div>false:</div><div>  %phi = i16 [ %a, ... ], [ %sub.conv, %true ]</div><div>--------</div></div><div><br></div><div style>The first one got if-converted by SimplifyCFG, the second one didn't. The promotion to i32 happened because of code elsewhere, it happened sometimes and not others, sometimes because of inlining. And the code is then handled by the rest of the optimizer completely differently.</div>
<div style><br></div><div style>The presence of a 'trunc' in the conditional basic block shouldn't be the thing that enables or disables this. All I wanted out of this patch was improved stability and consistency on x86.</div>
<div style><br></div><div style><br></div><div style>Now, why not just nuke the if-conversion altogether? Because, much to my regret, early if-conversion isn't enabled on x86, so we don't end up fixing this later. To make matters worse, other *IR* level passes were written assuming these selects would be formed... something I find very unfortunate.</div>
<div style><br></div><div style>Dan and I have been discussing various different approaches to solving the fundamental problem here and avoiding any such hacks, but I think that the badness you're arguing against is badness that was *already here*, and which we don't yet have a good alternative for. We should absolutely find a better way, but I don't think that should preclude some bandaids going in in the interim.</div>
<div style><br></div><div style><br></div><div style>I also added a big comment to the top of the function to try to warn people away from spending too much time growing more functionality here until we have a more principled solution in place. (I checked the LNT bots and saw no regressions from my work fwiw...)</div>
<div><br></div></div></div></div>