RFR: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late [v2]

[Christian Hagedorn]

On Mon, 23 Aug 2021 15:13:14 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix wrong assumption of single pin to uncommon projection & add tests which failed before
>
> src/hotspot/share/opto/loopPredicate.cpp line 254:
> 
>> 252:     _igvn.register_new_node_with_optimizer(clone);
>> 253:     old_new->map(next->_idx, clone);
>> 254:     if (next->in(0) == uncommon_proj) {
> 
> Doesn't that assume only one node in the chain has its control set? Wouldn't testing something like get_ctrl(next) != uncommon_proj be more robust?

Nice catch! This assumption is indeed wrong. I played around with some `Div` nodes and the algorithm bails out in step 2 too early when comparing for `next->in(0) == uncommon_proj` without doing a rewiring. This leads to a bad graph again. 

I think this check here in step 1 can be completely removed due to already checking with `get_ctrl()` below. Step 2 needs to be adapted such that we rewire the control input of a data nodes but also apply the rewiring process for such nodes with a control input within a chain. I pushed an update.

> src/hotspot/share/opto/loopPredicate.cpp line 283:
> 
>> 281:           assert(!in->is_CFG(), "must be data node");
>> 282:           Node* in_clone = old_new->at(in->_idx);
>> 283:           if (in_clone != NULL && in_clone->_idx >= last_idx) {
> 
> Doesn't in_clone != NULL implies in_clone->_idx >= last_idx? Should it be an assert?

I'm not sure because `old_new` is shared in loopopts. I was concerned that I find old mappings here which I tried to exclude.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5185