RFR: 8305634: Renaming predicates, simple cleanups, and adding summary about current predicates [v2]

[Christian Hagedorn]

On Wed, 10 May 2023 08:09:26 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix summary
>
> src/hotspot/share/opto/loopPredicate.cpp line 44:
> 
>> 42: /*
>> 43:  * The general idea of Loop Predication is to insert a predicate on the entry
>> 44:  * path to a loop, and raise a uncommon trap if the check of the condition fails.
> 
> Maybe say `uncommon trap` for runtime checks (ok if they fail), and `HaltNode` for assertion predicates (cannot fail)?

I've reworded this paragraph to only reflect the idea of Loop Predication to keep things simple.

> src/hotspot/share/opto/loopPredicate.cpp line 73:
> 
>> 71:  *                               predicate is created during Loop Predication and is inserted above the Profiled Loop
>> 72:  *                               Parse Predicate.
>> 73:  * - Loop Limit Check Predicate: This predicate is created when transforming a loop to a counted loop. It does not replace
> 
> What does it do? Check that the counter does never overflow?

I've added a short description.

> src/hotspot/share/opto/loopPredicate.cpp line 82:
> 
>> 80:  *                      Parse and Assertion Predicates are always removed before code generation (except for Initialized
>> 81:  *                      Assertion Predicates which are kept in debug builds while being removed in product builds).
>> 82:  * - Regular Predicate: This term is used to refer to a Runtime Predicate or a Parse Predicate and can be used to
> 
> Is this exactly the class of predicates that lead to uncommon traps?
> You could consider making this list more hierarchical. Name superclasser first.

Good idea, I've rearranged the terms to better reflect the hierarchy.

> src/hotspot/share/opto/loopPredicate.cpp line 93:
> 
>> 91:  *                        away to avoid a broken graph. Assertion Predicates are left in the graph as a sanity checks in
>> 92:  *                        debug builds (they must never fail at runtime) while they are being removed in product builds.
>> 93:  *                        We use special Opaque4 nodes to block some optimizations and replace the Assertion Predicates
> 
> Have you considered giving `Opaque4` a more descriptive name?

Yes, in later commits, I change that name to `OpaqueAssertionPredicateNode`. I could have already done this change here.

> src/hotspot/share/opto/loopPredicate.cpp line 155:
> 
>> 153:  *                            (JDK-8288981).
>> 154:  * - Regular Predicate Block: A Regular Predicate Block consists of a Parse Predicate a Runtime Predicate Block (all its
>> 155:  *                            Runtime Predicates, if any). There are three such blocks:
> 
> A Regular Predicate Block consists of a Runtime Predicate Block (all its Runtime Predicates, if any) with a Parse Predicate after it.

I've extended the text and switched their order.

> src/hotspot/share/opto/loopPredicate.cpp line 456:
> 
>> 454: IfProjNode* PhaseIdealLoop::clone_parse_predicate_to_unswitched_loop(ParsePredicateSuccessProj* predicate_proj,
>> 455:                                                                      Node* new_entry, Deoptimization::DeoptReason reason,
>> 456:                                                                      const bool slow_loop) {
> 
> Why not just one argument per line?

I've tried wrap at 120 characters.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189898040
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189848295
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189848738
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189850859
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189868295
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189878876