RFR: 8305634: Renaming predicates, simple cleanups, and adding summary about current predicates [v2]
Emanuel Peter
epeter at openjdk.org
Wed May 10 09:41:31 UTC 2023
On Mon, 8 May 2023 14:43:20 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:
>> This is the first patch in a series of patches to fix the remaining issues with Assertion/Skeleton Predicates ([JDK-8288981](https://bugs.openjdk.org/browse/JDK-8288981)). To achieve this task, we've decided to cleanup the naming scheme of predicates, cleanup the code operating on predicates (matching predicates, skipping over predicates etc.), and finally redesign the Assertion/Skeleton Predicates. While the basic idea stays the same, we are using new nodes and unified, cleaned up classes and methods to do the job. This redesign simplifies and only even makes it possible to fix the remaining known issues in a clean way.
>>
>> To make reviewing the entire change easier, I've decided to split the work into several PRs.
>>
>> This first PR includes the following _semantic-preserving_ changes:
>> - Establishing a new naming scheme for all the predicates found in C2 which makes it easier to talk about the various kinds of predicates:
>> - Updating the code (variables, method names etc.) accordingly.
>> - Renaming "Skeleton Predicates" to "Assertion Predicates".
>> - Including a summary of all predicates found in C2 in `loopPredicate.cpp`.
>> - Capitalizing predicate names to better distinguish them in comments (e.g. "Parse Predicate" instead of "parse predicate").
>> - Change `class Predicates` -> `class ParsePredicates`.
>> - Improving type information (e.g. using `IfProjNode` instead of `ProjNode`, using `ParsePredicateSuccessProj/ParsePredicateUncommonProj` typedefs for Parse Predicates etc.).
>> - Removing unused variables.
>> - Removing unnecessary checks.
>> - Code style fixes in touched code.
>>
>> Instead of giving more background information about Assertion Predicates and why we need them here, I've decided to write a _blog post_ dedicated to Assertion Predicates and Loop Predication. This provides an overview and introduction which, hopefully, makes reviewing the PRs related to Assertion Predicates easier - at least on a higher level.
>>
>> The blog post can be found on my Github page at:
>> https://chhagedorn.github.io/jdk/2023/05/05/assertion-predicates.html
>>
>> Thanks to @rwestrel, @eme64, and @TobiHartmann for your help with discussions, brainstormings, and pre-reviewing some of the changes here and in upcoming PRs.
>>
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>
> Fix summary
Nice work! The blog was a pleasure to read. I left a few comments.
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)?
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?
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.
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?
src/hotspot/share/opto/loopPredicate.cpp line 101:
> 99: * This predicate does not represent an actual check, yet, and
> 100: * just serves as a template to create an Initialized Assertion
> 101: * Predicate from for a (sub) loop.
Suggestion:
* Predicate for a (sub) loop.
src/hotspot/share/opto/loopPredicate.cpp line 112:
> 110: * - Loop Predication: A range check inside a loop is replaced by a Hoisted Predicate before
> 111: * the loop. We add two additional Template Assertion Predicates which
> 112: * are later used to create Initialized Assertion Predicates from. One
Suggestion:
* are later used to create Initialized Assertion Predicates. One
Or:
`We add two additional Template Assertion Predicates from which we can later create the Initialized Assertion Predicates.`
src/hotspot/share/opto/loopPredicate.cpp line 137:
> 135: * equal. The Initialized Assertion Predicates are always true because
> 136: * their range is covered by a corresponding Hoisted Predicate.
> 137: * - Range Check Elimination: A range check is removed from the main-loop by changing the pre
Optional: Mention why Loop Predication does not cover this case.
src/hotspot/share/opto/loopPredicate.cpp line 145:
> 143: * OpaqueLoop* nodes by actual values for the unrolled loop.
> 144: * The Initialized Assertion Predicates are always true because their
> 145: * range is covered by the main-loop entry guard.
As discussed in person: reason is pre and main exit condition.
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.
src/hotspot/share/opto/loopPredicate.cpp line 167:
> 165: * ... | Block | Loop Predicate Block
> 166: * [Loop Hoisted Predicate n + 2 Template Assertion Predicates] / |
> 167: * Loop Parse Predicate /
Suggestion:
* Loop Parse Predicate /
src/hotspot/share/opto/loopPredicate.cpp line 182:
> 180: * and applying Range Check Elimination (the order is insignificant):
> 181: *
> 182: * Main Loop entry guard
Suggestion:
* Main Loop entry (zero-trip) guard
src/hotspot/share/opto/loopPredicate.cpp line 182:
> 180: * and applying Range Check Elimination (the order is insignificant):
> 181: *
> 182: * Main Loop entry guard
Suggestion:
* Main Loop entry (zero-trip) guard
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?
-------------
PR Review: https://git.openjdk.org/jdk/pull/13864#pullrequestreview-1420018645
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189516154
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189556637
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189559673
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189562771
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189563920
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189569192
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189575199
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189591532
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189606227
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189594084
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189611843
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189612071
PR Review Comment: https://git.openjdk.org/jdk/pull/13864#discussion_r1189613598
More information about the hotspot-compiler-dev
mailing list