[PATCH] Select Elimination in InstCombine

[Gerolf Hoflehner]

Sure. Optional is a compromise:

My understanding is that there is a balance to strike with JITs that avoid analysis to reduce compile-time overhead. So every time a pass requires an analysis that a JIT does not provide the pass can’t be used.  For example, 4 MCJIT tests fail because the combiner pass is not initialized when DT is required.  I think ideally we need a mechanism like ‘when we can run the analysis it is required, otherwise optional’. 

Under normal circumstance DT information is available all combiner optimizations will kick in. When that assumption is broken the select-cmp-br.ll test cases will fail and at least we catch the issue.

-Gerolf





On Sep 9, 2014, at 2:55 PM, Juergen Ributzka <juergen at apple.com> wrote:

> The problem with making DT optional is that you might never get it. If any pass that runs before InstCombine invalidates DT, it won’t be available to any following pass until there is a pass that explicitly requires it.
> 
> -Juergen
> 
>> On Sep 9, 2014, at 2:50 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>> 
>> Ok, makes sense. The DT is optional and I added the missing check. When it is absent we just don’t recognize as many patterns.
>> 
>> I expect  DT usage to evolve over time, but have no specifics plans.
>> 
>> -Gerolf
>> 
>> On Sep 9, 2014, at 12:52 AM, Eric Christopher <echristo at gmail.com> wrote:
>> 
>>> Hi Gerolf,
>>> 
>>> One more comment in line.
>>> 
>>> As far as the dominance tree, you don't ever check to find out if you have dominance trees, you just require it. The leading question was: if it's not optional, then you can probably use it in more places in the pass. If it is optional then you're not actually checking for it before using it.
>>> 
>>> Thanks!
>>> 
>>> ================
>>> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2977
>>> @@ +2976,3 @@
>>> +              // replace select with operand on then path for NE compares.
>>> +              int Path = I.getPredicate() == ICmpInst::ICMP_EQ ? 1 : 0;
>>> +              if (InstCombiner::dominatesAllUses(
>>> ----------------
>>> majnemer wrote:
>>>> echristo wrote:
>>>>> Isn't this just a boolean then?
>>>> I'd make this an unsigned, getSuccessor is asking which successor to take.  It just so happens that we are only interested in cases where there are two successors.
>>> Yes. Makes sense that way. Might be better off just grabbing the correct successor rather than mucking with the integer.
>> I agree.
>>> 
>>> http://reviews.llvm.org/D5258
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140909/01328e35/attachment.html>