RFR: 8310308: IR Framework: check for type and size of vector nodes [v30]
Christian Hagedorn
chagedorn at openjdk.org
Mon Aug 14 13:57:29 UTC 2023
On Mon, 14 Aug 2023 11:17:29 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> For some changes to `SuperWord`, and maybe auto-vectorization in general, I want to strengthen the IR Framework.
>>
>> **Motivation**
>> I want to not just find the relevant IR nodes, but also assert that they have the maximal length that they could have on the respective platform (given the CPU features and `MaxVectorSize`). Without this verification it is possible that a future change leads to a regression where we still vectorize but at shorter vector widths as before - leading to performance loss.
>>
>> **How to use it**
>>
>> All `IRNode`s in `test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java` that are created with `vectorNode` are now all matched with their `type` and `size`. The regex might now look something like this:
>>
>> `"(\d+(\s){2}(VectorCastF2X.*)+(\s){2}===.*vector[A-Za-z][8]:{int})"`
>> which would match with IR nodes dumped like that:
>> `1150 VectorCastF2X === _ 1151 [[ 1146 ]] #vectory[8]:{int} ...`
>>
>> The goal was to keep it simple and straight forward. In most cases, you can just use the nodes as before, and implicitly we now check for maximal size automatically. However, in some cases we want to ensure there is no or only a limited number of nodes (`failOn` or comparison `<` or `<=` or `=0`) - in those cases we usually want to make sure there is not any node of any size, so we match with any size by default. The size can also explicitly be constrained using `IRNode.VECTOR_SIZE`.
>>
>> Some examples:
>> 1. `@IR(counts = {IRNode.LOAD_VECTOR_I, " >0 "})` -> search for a `LoadVector` node with `type` `int`, and maximal `size` possible on the machine (limited by CPU features and `MaxVectorSize`). This is the most common use case.
>> 2. `@IR(failOn = { IRNode.LOAD_VECTOR_L, IRNode.STORE_VECTOR })` -> fail if there is a `LoadVector` with type `long`, of `any` size.
>> 3. `@IR(counts = { IRNode.XOR_VI, IRNode.VECTOR_SIZE_4, " > 0 "})` -> find at least one `XorV` node with type `int` and exactly `4` elements. Useful for VectorAPI when the vector species is fixed.
>> 4. `@IR(counts = { IRNode.LOAD_VECTOR_D, IRNode.VECTOR_SIZE + "min(4, max_double)", " >0 " })` -> search for a `LoadVector` node with `type` `double`, and `size` exactly equals to `min(4, max_double)` (so 4 elements, or if the hardware allows fewer `doubles`, then that number).
>> 5. `@IR(counts = { IRNode.ABS_VF, IRNode.VECTOR_SIZE + "min(LoopMaxUnroll, max_float)", ">= 1" })` -> find at least one `AbsV` nodes with type `float`, and the `size` exactly equals to the smaller of...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>
> Fix comments according to TobiHartmann
I have only some minor comments left, otherwise, the update looks good to me!
test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 113:
> 111: public static final String VECTOR_SIZE_TAG_ANY = "any";
> 112: public static final String VECTOR_SIZE_TAG_MAX = "max_for_type";
> 113: public static final String VECTOR_SIZE_ANY = VECTOR_SIZE + VECTOR_SIZE_TAG_ANY; // default for count "=0" and failOn
Suggestion:
public static final String VECTOR_SIZE_ANY = VECTOR_SIZE + VECTOR_SIZE_TAG_ANY; // default for counts "=0" and failOn
test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 114:
> 112: public static final String VECTOR_SIZE_TAG_MAX = "max_for_type";
> 113: public static final String VECTOR_SIZE_ANY = VECTOR_SIZE + VECTOR_SIZE_TAG_ANY; // default for count "=0" and failOn
> 114: public static final String VECTOR_SIZE_MAX = VECTOR_SIZE + VECTOR_SIZE_TAG_MAX; // default in count
Suggestion:
public static final String VECTOR_SIZE_MAX = VECTOR_SIZE + VECTOR_SIZE_TAG_MAX; // default in counts
test/hotspot/jtreg/compiler/lib/ir_framework/driver/SuccessOnlyConstraintException.java line 28:
> 26: /**
> 27: * Exception used to signal that the Constraint should always suceed.
> 28: */
Suggestion:
import compiler.lib.ir_framework.driver.irmatching.irrule.constraint.Constraint;
/**
* Exception used to signal that a {@link Constraint} should always succeed.
*/
test/hotspot/jtreg/compiler/lib/ir_framework/driver/SuccessOnlyConstraintException.java line 31:
> 29: public class SuccessOnlyConstraintException extends RuntimeException {
> 30: public SuccessOnlyConstraintException(String message) {
> 31: super("Unhandled SuccessOnlyConstraintException, should have created a Constraint that alway suceeds:" + System.lineSeparator() + message);
Suggestion:
super("Unhandled SuccessOnlyConstraintException, should have created a Constraint that always succeeds:" + System.lineSeparator() + message);
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/irrule/checkattribute/parsing/RawIRNode.java line 92:
> 90: if (!userPostfix.isValid() || !vmInfo.canTrustVectorSize()) {
> 91: switch (bound) {
> 92: case Comparison.Bound.LOWER -> {
Suggestion:
case LOWER -> {
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/irrule/checkattribute/parsing/RawIRNode.java line 105:
> 103: }
> 104: }
> 105: case Comparison.Bound.UPPER -> {
Suggestion:
case UPPER -> {
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/irrule/checkattribute/parsing/RawIRNode.java line 120:
> 118: }
> 119: }
> 120: case Comparison.Bound.EQUAL -> {
Suggestion:
case EQUAL -> {
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/irrule/checkattribute/parsing/RawIRNode.java line 127:
> 125: // Equal comparison to a strictly positive number would lead us to an impossible
> 126: // situation: we might have to know the exact vector size or else we count too many
> 127: // or too few cases. Because of this we forbid this case in general.
The comment is outdated and could, for example, be updated to:
Suggestion:
// or too few cases. We therefore skip such a constraint and treat it as success.
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/irrule/constraint/SuccessConstraintCheck.java line 28:
> 26: import compiler.lib.ir_framework.IR;
> 27: import compiler.lib.ir_framework.driver.irmatching.MatchResult;
> 28: import compiler.lib.ir_framework.shared.Comparison;
Suggestion:
import compiler.lib.ir_framework.driver.irmatching.MatchResult;
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/irrule/constraint/SuccessConstraintCheck.java line 38:
> 36: */
> 37: class SuccessConstraintCheck implements ConstraintCheck {
> 38: public SuccessConstraintCheck() {}
This default constructor can be removed since it's implicitly added for us.
Suggestion:
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/parser/VMInfo.java line 67:
> 65: System.err.println(" -> SuperWord expected to run with 32 byte, not 64 byte, VectorAPI expected to use 64 byte");
> 66: System.err.println(" -> \"canTrustVectorSize == false\", some vector node IR rules are made weaker.");
> 67: }
Is this a leftover from debugging? If you want to print this information for debugging purposes, I suggest to move this code to `VMInfoParser` and additionally guard it with `VERBOSE || PRINT_IR_ENCODING`. The name `PRINT_IR_ENCODING` is not completely correct here but we might want to clean this up separately at some other point in time.
You can keep the verification of calling the `get*()` methods here, though.
test/hotspot/jtreg/compiler/lib/ir_framework/driver/irmatching/parser/VMInfo.java line 108:
> 106: * Some platforms do not behave as expected, and one cannot trust that the vectors
> 107: * make use of the full MaxVectorSize. For Cascade Lake we by default only use
> 108: * 32 bytes for SuperWord even though MaxVectorSize is 64. But the VectorAPI still
Suggestion: For Cascade Lake, we only use 32 bytes for SuperWord by default even though MaxVectorSize is 64.
-------------
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14539#pullrequestreview-1569244364
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293347002
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293347135
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293359212
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293361113
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293452244
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293452500
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293452691
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293455080
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293363923
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293457878
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293468601
PR Review Comment: https://git.openjdk.org/jdk/pull/14539#discussion_r1293475850
More information about the hotspot-compiler-dev
mailing list