On 10/15/18 3:02 AM, Richard Biener wrote: > On Fri, Oct 12, 2018 at 10:01 PM Bill Schmidt <wschm...@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
