RFR: 8305638: Refactor Template Assertion Predicate Bool creation and Predicate code in Split If and Loop Unswitching

[Emanuel Peter]

On Fri, 22 Dec 2023 13:12:52 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/loopUnswitch.cpp line 117:
>> 
>>> 115: 
>>> 116: // Perform Loop Unswitching on the loop containing an invariant test that does not exit the loop. The loop is cloned
>>> 117: // such that we have two identical loops next to each other - a fast and a slow loop. We modify the loops as follows:
>> 
>> It sounds like they are somehow identical and at the same time modified.
>> Suggestion: say that they are "first" cloned to be identical, and then "second" modified such that one becomes the "true-path" and the other the "false-path" loop. I wonder if we can eliminate the "fast / slow" wording, because who really knows which path is faster or slower, right?
>
> Also: why can the invariant if not be eliminated in the loops as a dominating if, with and independent optimization? Would that not be cleaner? Well I guess maybe we just want to be sure that it happens.

Yeah, the naming of "old / new", "true / false", "fast / slow" is confusing and redundant. Would be nice if you said which are the same. Or maybe remove some of these names entirely.
I suggest: Old -> True, New -> False.

>> src/hotspot/share/opto/loopUnswitch.cpp line 266:
>> 
>>> 264:         _selector(create_selector_if(loop, unswitch_if_candidate)),
>>> 265:         _fast_loop_proj(create_fast_loop_proj()),
>>> 266:         _slow_loop_proj(create_slow_loop_proj()) {}
>> 
>> It seems to me you could make all fields `const` of some sort, right?
>
> You will never modify the pointers again.

Just for good measure: assert that the `unswitch_if_candidate` is inside the `loop`?

>> src/hotspot/share/opto/loopUnswitch.cpp line 325:
>> 
>>> 323:         _loop(loop),
>>> 324:         _old_new(old_new),
>>> 325:         _phase(loop->_phase) {}
>> 
>> Again, I think it would be nice to have the constructor and public member methods at the beginning, right after the field definition. It just helps to see how they belong together better.
>
> And the fields could be const.

And hence the methods could be const as well

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435061059
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435107903
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435132160