[PATCH] Fix PR87473 (SLSR ICE on hidden basis)

[Jeff Law]

On 10/15/18 3:02 AM, Richard Biener wrote:
> On Fri, Oct 12, 2018 at 10:01 PM Bill Schmidt <wschmidt@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> This patch addresses SLSR bug PR87473.  The underlying problem here is that
>> create_add_on_incoming_edge contains code to handle a phi_arg that is equal
>> to the base expression of the PHI candidate, where the increment assigned to
>> the incoming arc should be zero minus the index expression of the hidden
>> basis; but several other places in SLSR processing need to handle the same
>> case, and fail to do so.  As a result, the code to replace the PHI basis
>> attempts to use an initializing statement that was never created in the first
>> place, and we ICE.  This patch adds the necessary logic in four parts of the
>> code to ensure we handle this consistently throughout.
>>
>> This error survived this long because the typical case when accessing the
>> hidden basis is for the index of the hidden basis to be zero.  For such a
>> case we don't need an initializing statement, and the ICE doesn't trigger.
>> The test case provided with the PR is a counter-example where the hidden
>> basis has an index of 2.
>>
>> For the four functions fixed here, each identified the case of interest,
>> but just didn't do anything when that case arose.  I've reorganized the
>> code in each case to always execute the relevant logic, but change what's
>> done for the specific situation of the "pass-through" PHI argument.  This
>> makes the diffs a little hard to read, unfortunately.
>>
>> During the investigation I noticed that we are dealing with a degenerate PHI,
>> introduced by the loopinit pass, that we would be better off optimizing away
>> sooner:
>>
>>  <bb 5> [local count: 14598063]:
>>   # qz_1 = PHI <qz_3(4)>
>>   # jl_22 = PHI <jl_6(4)>
>>   _8 = (unsigned int) jl_22;
>>   _13 = _8 * _15;
>>   qz_11 = (int) _13;
>>
>> The assignment to _8 should just use jl_6 in place of jl_22.  This would
>> greatly simplify SLSR's job, since PHI-free code is handled much more
>> straightforwardly than code that involves conditional updates.  We go through
>> at least 30 passes without having this cleaned up, and I expect other passes
>> than SLSR would perhaps be hamstrung by this as well.  Any recommendations?
> 
> Without more context these are likely loop-closed PHIs.  It's probaby DOM
> after loop that gets rid of them currently but a cheaper way would be to
> propagate them out in pass_tree_loop_done.  Note that IIRC there are some
> other passes rewriting things into loop-closed SSA form that might expose
> such degenerate PHIs as well (a quick look shows invariant motion, both
> VRP and EVRP should eventually propagate them out during their propagation
> step and EVRP shouldn't even need loop-closed SSA?).
There's phi_only_cprop which can deal with this stuff very fast.  It
creates a worklist of degenerate PHIs, propagates them away, adding
anything that becomes a degenerate (including normal statements) to the
worklist.  We typically run it after the dom/vrp or vrp/dom pass pairs
because threading tends to create lots of these degenerates.


The question I would answer is where does the PHI first appear and when
does it become a degenerate?

jeff