[lworld] RFR: [DRAFT] [lworld] Test scalarization and call conventions for different value class shapes

[Emanuel Peter]

On Fri, 15 Aug 2025 09:25:32 GMT, Galder Zamarreño <galder at openjdk.org> wrote:

> This is a **very early draft** for a template-based test (temporarily called `TestOne`) that addresses some of the requirements of JDK-8364421.
> 
> **NOTE**: the PR also includes the commits to add template-based testing framework since that's not yet in `lworld`, and also adds https://github.com/openjdk/valhalla/commit/68e4a60c1ff62f4d0b8b31cb0bd729f1d67748e2 so that Boolean types are supported in `checkEQ`.
> 
> This test has surfaced [JDK-8365590](https://bugs.openjdk.org/browse/JDK-8365590) bug, and as I focus on that bug, I thought it'd be a good moment to share what I've done so far with the test and get some feedback.
> 
> The test is very simple so far, creating value classes that contain a single field, testing for each of the primitive types. I also started testing having this field be a primitive array type and that's when I encountered [JDK-8365590](https://bugs.openjdk.org/browse/JDK-8365590).
> 
> I've tried to create value classes that had multiple fields, but I've hit some obstacles here so I've just added a very basic test:
> 
> First, I tried to use data names to create value classes that would have N fields, and have these fields initialized via a constructor, but I couldn't get it to work. So the example present in the test uses external data structures to keep track of these fields, their types, their initial values...etc. I have discussed these issues in more detail with @chhagedorn and bounced some ideas of how to improve data names, but these need to be discussed in more detail.
> 
> The second obstacle I found is related to nuances on when value classes with multiple fields will be scalarized, when these classes are part of another value classes. Depending on the combination of fields, they can be considered that they can be flattened or not, so handling this in a test like this would need to track the sizes of each fields, add them up, and decide whether the test should be positive on scalarization or negative. I traced this logic to `FieldLayoutBuilder::compute_inline_class_layout` and the way `_nullable_layout_size_in_bytes` is then used to decide whether to flatten or not. I've been using `PrintFieldLayout` option to see what nullable flat layout values each of these box instances would have.
> 
> So, given these complexities of multiple fields, I'm might keep `TestOne` (note: name is temporary) focused on value classes with a single field. And then have another test with multiple fields.

I know this is in draft state. But I thought I'd have a look since you are trying to use the template framework. That's exciting :)

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 104:

> 102:         String name() {
> 103:             return "v" + id;
> 104:         }

What is the purpose of the `id` here? Do you ever need it, or just the `name()`?

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 122:

> 120:         return Template.make("FIELD", (FieldConstant f) -> body(
> 121:             let("FIELD_TYPE", f.type),
> 122:             let("FIELD_VALUE", f.value),

Is this the only place where you use the `value` of `FieldConstant`? Why not just get a `field.type.con()` right here? Maybe I'm missing some part of the design here.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 172:

> 170:         final List<FieldConstant> fields = fieldTypes.stream()
> 171:             .map(fieldType -> FieldConstant.of(fieldId.getAndIncrement(), fieldType))
> 172:             .toList();

Why do you need the constants associated with the fields here?

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 178:

> 176:             static value class $Box {
> 177:             """,
> 178:             fields(fields),

Wow, I did not know that one could have both a local variable and a method with the same name! Even if Java lets you do it, it seems a bit confusing to me 😅 Maybe call the method `generateFields`?

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 210:

> 208:             """
> 209:                 0;
> 210:             }

You should probably rather joint a list of strings with `" + "`, rather than letting the `hashField` add the `+` that may feel a little lost there 😅

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 223:

> 221:             """
> 222:         )).asToken(field);
> 223:     }

Looks like you don't actually need any arguments in the Template here. You are already capturing `field` in the lambda.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/templating/TestOne.java line 247:

> 245:             return name();
> 246:         }
> 247:     }

We may want to have some sort of array type in the mainline library as well.

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

PR Review: https://git.openjdk.org/valhalla/pull/1522#pullrequestreview-3128424463
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282485655
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282493883
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282462905
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282500052
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282450236
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282440797
PR Review Comment: https://git.openjdk.org/valhalla/pull/1522#discussion_r2282433500