RFR: 8359412: Template-Framework Library: Operations and Expressions [v6]
Christian Hagedorn
chagedorn at openjdk.org
Mon Oct 13 08:15:23 UTC 2025
On Thu, 18 Sep 2025 08:12:50 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> Impliementing ideas from original draft PR: https://github.com/openjdk/jdk/pull/23418 ([Exceptions](https://github.com/openjdk/jdk/pull/23418/files#diff-77e7db8cc0c5e02786e1c993362f98fabe219042eb342fdaffc09fd11380259dR41), [ExpressionFuzzer](https://github.com/openjdk/jdk/pull/23418/files#diff-01844ca5cb007f5eab5fa4195f2f1378d4e7c64ba477fba64626c98ff4054038R66)).
>>
>> Specifically, I'm extending the Template Library with `Expression`s, and lists of `Operations` (some basic Expressions). These Expressions can easily be nested and then filled with arguments, and applied in a `Template`.
>>
>> Details, in **order you should review**:
>> - `Operations.java`: maps lots of primitive operators as Expressions.
>> - `Expression.java`: the fundamental engine behind Expressions.
>> - `examples/TestExpressions.java`: basic example using Expressions, filling them with random constants.
>> - `tests/TestExpression.java`: correctness test of Expression machinery.
>> - `compiler/igvn/ExpressionFuzzer.java`: expression fuzzer for primitive type expressions, including input range/bits constraints and output range/bits verification.
>> - `PrimitiveType.java`: added `LibraryRNG` facility. We already had `type.con()` which gave us random constants. But we also want to have `type.callLibraryRNG()` so that we can insert a call to a random number generator of the corresponding primitive type. I use this facility in the `ExpressionFuzzer.java` to generate random arguments for the expressions.
>> - `examples/TestPrimitiveTypes.java`: added a `LibraryRNG` example, that tests that has a weak test for randomness: we should have at least 2 different value in 1000 calls.
>>
>> If the reviewers absolutely insist, I could split out `LibraryRNG` into a separate RFE. But it's really not that much code, and has direct use in the `Expression` examples.
>>
>> **Future Work**:
>> - Use `Expression`s in a loop over arrays / MemorySegment: fuzz auto-vectorization.
>> - Use `Expression`s to model more operations:
>> - `Vector API`, more arithmetic operations like from `Math` classes etc.
>> - Ensure that the constraints / checksum mechanic in `compiler/igvn/ExpressionFuzzer.java` work, using IR rules. We may even need to add new IGVN optimizations. Add unsigned constraints.
>> - Find a way to delay IGVN optimizations to test worklist notification: For example, we could add a new testing operator call `TestUtils.delay(x) -> x`, which is intrinsified as some new `DelayNode` that in normal circumstances just fol...
>
> Emanuel Peter has updated the pull request incrementally with two additional commits since the last revision:
>
> - more comments
> - add othervm to test
Nice work! I left some comments here and there when walking through the code but I did not deep dive into it much since it was already properly reviewed by 2 reviewers :-)
test/hotspot/jtreg/compiler/lib/template_framework/library/Expression.java line 44:
> 42: *
> 43: * <p>
> 44: * The {@link Expression}s are composable, they can be explicitly {@link nest}ed, or randomly
Suggestion:
* The {@link Expression}s are composable, they can be explicitly {@link #nest}ed, or randomly
test/hotspot/jtreg/compiler/lib/template_framework/library/Expression.java line 349:
> 347: " expected: " + argumentTypes.size() +
> 348: " but got: " + arguments.size() +
> 349: " for " + this.toString());
`toString()` is implicitly called:
Suggestion:
" for " + this);
test/hotspot/jtreg/compiler/lib/template_framework/library/Expression.java line 401:
> 399: List<Expression> filtered = expressions.stream().filter(e -> e.returnType.isSubtypeOf(returnType)).toList();
> 400:
> 401: if (filtered.size() == 0) {
Suggestion:
if (filtered.isEmpty()) {
test/hotspot/jtreg/compiler/lib/template_framework/library/Expression.java line 426:
> 424: List<Expression> filtered = nestingExpressions.stream().filter(e -> e.returnType.isSubtypeOf(argumentType)).toList();
> 425:
> 426: if (filtered.size() == 0) {
Suggestion:
if (filtered.isEmpty()) {
test/hotspot/jtreg/compiler/lib/template_framework/library/Expression.java line 460:
> 458: newStrings.add(this.strings.get(argumentIndex) +
> 459: nestingExpression.strings.get(0)); // concat s1 and S0
> 460: newArgumentTypes.add(nestingExpression.argumentTypes.get(0));
You can use `getFirst()`:
Suggestion:
nestingExpression.strings.getFirst()); // concat s1 and S0
newArgumentTypes.add(nestingExpression.argumentTypes.getFirst());
test/hotspot/jtreg/compiler/lib/template_framework/library/Operations.java line 67:
> 65: ops.add(Expression.make(BYTES, "(byte)(", LONGS, ")"));
> 66: ops.add(Expression.make(BYTES, "(byte)(", FLOATS, ")"));
> 67: ops.add(Expression.make(BYTES, "(byte)(", DOUBLES, ")"));
There is a lot of repetition of this block for the various types. Could you share the code? Maybe something like this:
ops.add(Expression.make(returnType, "(castType)(", BYTES, ")"));
ops.add(Expression.make(returnType, "(castType)(", SHORTS, ")"));
ops.add(Expression.make(returnType, "(castType)(", CHARS, ")"));
ops.add(Expression.make(returnType, "(castType)(", INTS, ")"));
ops.add(Expression.make(returnType, "(castType)(", LONGS, ")"));
ops.add(Expression.make(returnType, "(castType)(", FLOATS, ")"));
ops.add(Expression.make(returnType, "(castType)(", DOUBLES, ")"));
test/hotspot/jtreg/compiler/lib/template_framework/library/Operations.java line 72:
> 70: ops.add(Expression.make(BYTES, "(", BOOLEANS, "?", BYTES, ":", BYTES, ")"));
> 71:
> 72: // Arithmetic operations are not performned in byte, but rather promoted to int.
Suggestion:
// Arithmetic operations are not performed in byte, but rather promoted to int.
test/hotspot/jtreg/compiler/lib/template_framework/library/Operations.java line 154:
> 152: ops.add(Expression.make(BOOLEANS, "(", INTS, " < ", INTS, ")"));
> 153: ops.add(Expression.make(BOOLEANS, "(", INTS, " >= ", INTS, ")"));
> 154: ops.add(Expression.make(BOOLEANS, "(", INTS, " <= ", INTS, ")"));
This also seems to be repeated for `LONGS` and could be shared.
test/hotspot/jtreg/compiler/lib/template_framework/library/Operations.java line 263:
> 261: ops.add(Expression.make(BOOLEANS, "(", FLOATS, " < ", FLOATS, ")"));
> 262: ops.add(Expression.make(BOOLEANS, "(", FLOATS, " >= ", FLOATS, ")"));
> 263: ops.add(Expression.make(BOOLEANS, "(", FLOATS, " <= ", FLOATS, ")"));
Could also be shared with the double version.
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java line 180:
> 178: // Fill an array with 1_000 random values. Every type has at least 2 values,
> 179: // so the chance that all values are the same is 2^-1_000 < 10^-300. This should
> 180: // never happen, even with a relatively weak PRNG.
And if it does, we should consider playing the lottery :-)
test/hotspot/jtreg/testlibrary_tests/template_framework/examples/TestPrimitiveTypes.java line 226:
> 224: import java.util.Random;
> 225: import jdk.test.lib.Utils;
> 226: import compiler.lib.generators.*;
Could this be an utility method like `libraryRNGImports()` or something like that?
-------------
Marked as reviewed by chagedorn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26885#pullrequestreview-3330339890
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425503658
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425509576
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425506121
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425506497
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425508014
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425480526
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425471097
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425485743
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425488296
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425494544
PR Review Comment: https://git.openjdk.org/jdk/pull/26885#discussion_r2425497241
More information about the hotspot-compiler-dev
mailing list