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

[Roland Westrelin]

On Fri, 27 Aug 2021 07:07:50 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> src/hotspot/share/opto/loopPredicate.cpp line 192:
>> 
>>> 190:   assert(rgn->in(rgn->req() -1) == if_uct, "new edge should be last");
>>> 191:   bool has_phi = false;
>>> 192:   bool unswitch_clone_for_fast_loop = old_new != NULL && uncommon_proj->outcnt() > 1;
>> 
>> Isn't it inconsistent that you compute unswitch_clone_for_fast_loop here but pass a precomputed is_slow_loop when the expression looks almost identical: why compute one locally but pass the other one as an argument? Or am I missing something?
>
> I want to pass a tristate: [no unswitching, fast loop, slow loop]. I initially had two bool flags passed to this function for that. But then I thought since `old_new` is only non-null in the fast loop case, I can use that one as "flag" to decide if we need to clone for the fast loop. I use `unswitch_is_slow_loop` as flag for the slow loop since `old_new` is `NULL` in this case. `false` and `NULL` is then the third state for no unswitching. But maybe it's better to introduce the second bool flag again to make it more clear.

It would seem a single variable that carries the tri state would be the clearest option. That would require an enum though so not sure if it's worth it. Anyway, I'm good with the change as it is. I only found the 2 boolean variables a bit confusing.

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

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