[llvm-commits] patch: moving unreachable elimination to codegenprep

[Nick Lewycky]

Dan Gohman wrote:
>
> On 2 February 2010 21:21, Nick Lewycky<nicholas at mxc.ca>  wrote:
>>
>> Jakob, I'll make the tweaks to InlineCost that you proposed and post an
>> updated patch with the results of a nightly test run. Realize that I'm
>> expecting no visible performance difference at all since no code in the
>> nightly test uses __builtin_unreachable().
>
> There is code which uses things like "noreturn" and other
> things which can lead to unreachable getting generated.
> Offhand you may be right that these wouldn't be affected
> though. This happens to be a very testable hypothesis :).

A 'noreturn' function call turns into a call + unreachable. The call 
can't be eliminated, so this patch has no effect on that case, since 
it's not a branch to unreachable (it'd a branch to call).

I want to emphasize that this patch is not nearly as scary as you seem 
to think it is. :-)

> Would you mind compiling some tests with and without the
> change, and diffing the .s files? I'd be interested in
> learning what kinds of diffs, if any, appear.

Done, asmchanges.diff attached. Original is - with my patch is + . I 
took TEST.llc.Makefile and modified it to keep the .s files, then did a 
run with and without the patch to see what happens to the output. 
Everything different was diff'd and all the differences that were 
irrelevant (timestamp, filename, anonymous namespace(!)) were removed. 
The remaining differences are a complete mystery to me -- I wonder 
whether two runs of llc even emit the same thing. :)

> I'm concerned that artificially keeping the condition
> instructions live will hamper the optimizer. Your benchmark
> results are good, however it's not clear if that's just a
> consequence of an absence of  __builtin_unreachable or if
> that actually says something about the impact of artificial
> life on the optimizer.
>
> One comment on the patch itself:
>
> --- lib/Analysis/ScalarEvolution.cpp    (revision 95201)
> +++ lib/Analysis/ScalarEvolution.cpp    (working copy)
>
> +  // Ignore faux exits.
> +  for (unsigned i = 0; i != ExitingBlocks.size(); ++i) {
> +    if (isa<UnreachableInst>(ExitingBlocks[i]->front())) {
> +      ExitingBlocks.erase(ExitingBlocks.begin() + i);
> +      --i;
> +    }
> +  }
>
> ExitingBlocks is an unsorted array. Instead of deleting blocks from
> the middle of it, please swap the element to the end and pop_back().

Good point. Thanks! Updated patch attached.

> Also, would it make sense to pull this code into the Loop(Base) class
> itself? There is other code that looks at loop exists and probably
> would need this.

I'm not sure. This stuff is used to look at the CFG and I don't really 
want it to start lying lest we end up breaking domtree updates or 
violating SSA properties. It would make sense to revisit this if all 
users are doing things that are safe when ignoring dead exits.

Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: asmchanges.diff
Type: text/x-diff
Size: 296077 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100215/b8930385/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: preserve-unreachable4.patch
Type: text/x-diff
Size: 23802 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100215/b8930385/attachment.patch>