[PATCH] Extend EarlyCSE to handle basic cases from JumpThreading and CVP

[Philip Reames]

On 05/19/2015 08:50 AM, Daniel Berlin wrote:
>> ================
>> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:471
>> @@ +470,3 @@
>> +  if (BasicBlock *Pred = BB->getSinglePredecessor())
>> +    // TODO: handle switch with case value as well
>> +    if (auto *BI = dyn_cast<BranchInst>(Pred->getTerminator()))
>> ----------------
>> Even GVN does not handle switch with case value, even when it can determine the case value as a constant. It is not as simple as one would thing because it's not about dominance, but control dependence.
>>
>>
>> (and DT->dominates will fail the single edge assertion in those cases, even though it's a reasonable thing to test)
>>
>>
>
> Just to explain this:
>
> getSinglePredecessor returns true if all edges lead to the same block.
> Thus, a switch with multiple edges to the same block will return a
> block to getSinglePredecessor.
Er, I believe this is incorrect.  The behavior you're describing is true 
of getUniquePredecessor, but not getSinglePredecessor.  This is why I'm 
explicitly using the getSinglePredecessor version.
>
> Dominates(Edge, anything) asserts isSingleEdge(Edge).
> isSingleEdge asserts that the edge is singular in the sense that
> "multiple edges do not exist from block EdgeStart to block EdgeEnd".
> Thus, getSinglePredecessor will happily give you blocks from switch
> statements that fail this test (any switch with case statements
> leading to the same block), such that you can't test dominance on
> them.
For getUniquePredecessor, I believe the problem you describe is relevant 
for branches too.  Nothing says a conditional branch can't have both 
it's destinations going to the same block.  The optimizer would likely 
fold such thing away quickly, but it is legal IR.
>
> GVN goes to the trouble of tracking how many edges lead to a target
> block, and only propagates equalities if a single edge reaches a given
> target block to avoid this assert.
>
> This is true *even if* it could prove that only one of the edges to
> the target block is reachable (because it would still hit the
> isSingleEdge assert).
If it can prove this, it should be breaking the other edges.  Once 
that's done, the assert holds.  I don't see the problem?
>
> At some point, that assert should simply be removed (or such was the
> conclusion of the folks who added it when i asked them, since the
> justification given for it makes no sense),
I'm not sure I actually agree here.  I don't actively disagree either, 
but I've found the assertion quite helpful in finding what would have 
otherwise been a subtle performance problem in one of my other changes.  
I hadn't *intended* to visit anything other than a single edge and had 
managed to phrase the query in a way where I did.
>   but it does mean that
> "handling switch with case value" is not a trivial TODO :)
>
> (note, of course, that splitting all critical edges also solves this
> particular problem)
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits