[12] RFR(S): 8208275: C2 crash in Node::add_req(Node*)

[Tobias Hartmann]

Hi Roland,

thanks for looking at this!

On 14.08.2018 14:33, Roland Westrelin wrote:
>> A loop with only one iteration is strip mined and then processed by
>> IdealLoopTree::policy_do_one_iteration_loop() which replaces the tripcount phi such that the
>> backedge becomes dead and the loop goes away. Usually, the code in RegionNode::Ideal [1] takes care
>> of removing the OuterStripMinedLoop as well but in the failing case the split-if optimization is
>> invoked first (it depends on the order in which igvn processes nodes in the worklist). Split-if now
>> performs its magic and then removes the CountedLoop without removing the OuterStripMinedLoop [2].
> 
> So an if is split through a counted loop head?

Yes but I think that's only (ever) possible because the backedge is essentially dead and the loop
phi was removed (as I've tried to explain below). If the loop still has a tripcount phi, split-if
will bail out because there's a phi node that is used by an AddI (or some other instruction that
increments/decrements it) or because other values are merged. I've tried for a long time to come up
with a way to trigger split-if for a strip mined loop in another way and I'm quite confident that
it's not possible.

> In general, that would break loop strip mining because it would cause
> some control flow to be inserted between the OuterStripMinedLoop and the
> new loop heads. And I think that's the root of the problem. 

No, the "new loop heads" are two regions with a single input each. Both are removed immediately by
IGVN because the backedge is dead. The root of the problem is that the dead CountedLoop is removed
without the OuterStripMinedLoop. Disabling split-if would generate the exact same IR except for the
OuterStripMinedLoop.

>> The straight forward fix is to remove the outer loop in policy_do_one_iteration_loop() similar to
>> what we do in PhaseIdealLoop::intrinsify_fill(). I was wondering if other optimizations could
>> trigger the same problem but came to the conclusion that split-if would bail out because either:
>> - the tripcount phi was removed and the counted loop backedge is already top [3] or
>> - the tripcount phi is still there and used by the AddI for increment [4]
> 
> That would not fix the root cause. It would fix the case where the
> counted loop is going away. But applying split if to a counted loop when
> the counted loop is not being optimized away would break.

Right but the split-if logic does not support this general case (see my explanation above). Or do
you see a way to trigger this?

>> An alternative would be to bail out of the split-if optimization if the region is a strip mined loop
>> or remove the outer strip mined loop if the region is removed.
> 
> Removing the outer strip mined loop entirely is not ok. There could
> still be loops after split if and that would make those loops run with
> no safepoint at all.

Yes, assuming that split-if would ever split through a strip mined loop that is not dead.

> Bailing out would be fine if that code never triggers with counted loops
> but the code has some tests for CountedLoop so it looks like it does
> (but it's strange that this never broke before).

Yes, I've seen that and I've assumed that those are remainders from a time when split-if supported
more cases (see for example the disabled code here [1] and below). In general, that code is really
old (Christmas '98 [2]). We could add an assert/guarantee to verify that it never splits through a
strip mined loop.

> I would say the best way to deal with this would be to transform the
> outer strip mined loop + counted loop back into the initial counted loop
> with a safepoint in the loop and then apply split if. Further loop opts
> passes would have a chance to build strip mined loop nests for the
> remaining loops.
> 
> The loop nest structure is:
> 
> OuterStripMinedLoop
> CountedLoop
> CountedLoopEnd
> SafePoint
> If // branches back to OuterStripMinedLoop
> 
> and we would to turn this into:
> 
> Region
> SafePoint
> If // branches back to OuterStripMinedLoop
> 
> by:
> 
> removing the OuterStripMinedLoop
> cloning the CountedLoop into a Region with the backedge of the
> OuterStripMinedLoop
> removing the CountedLoopEnd
> Changing the If condition to that of the CountedLoopEnd

That sounds like a lot of complexity for an optimization that is very rare (if possible at all) and
probably won't improve things much (taking loop specific optimizations into account).

Thanks,
Tobias

[1] http://hg.openjdk.java.net/jdk/jdk/file/38ec0cea438e/src/hotspot/share/opto/ifnode.cpp#l167
[2] http://hg.openjdk.java.net/jdk/jdk/file/38ec0cea438e/src/hotspot/share/opto/ifnode.cpp#l76