[DRAFT] [lworld] Test scalarization and call conventions for different value class shapes #1522
Conversation
Co-authored-by: Tobias Hartmann <[email protected]> Co-authored-by: Tobias Holenstein <[email protected]> Co-authored-by: Theo Weidmann <[email protected]> Co-authored-by: Roberto Castañeda Lozano <[email protected]> Co-authored-by: Christian Hagedorn <[email protected]> Co-authored-by: Manuel Hässig <[email protected]> Reviewed-by: chagedorn, mhaessig, rcastanedalo
…class Reviewed-by: chagedorn, mhaessig
Reviewed-by: mhaessig, chagedorn
…bjects Reviewed-by: chagedorn, thartmann
* The most basic of tests * Switch to more precise IRNode failure conditions * Disable EA, easily test with non-value class and add report stdout * Simplify and align with Box1/test1 * Use @check instead of @run * Make it public to access it from template * Port test over to the template framework * Parameterize box type
* But the bug still remains
|
👋 Welcome back galder! A progress list of the required criteria for merging this PR into |
|
@galderz This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
bot
added
ready
| static final class IntArrayType implements CodeGenerationDataNameType { | ||
| @Override | ||
| public Object con() { | ||
| return "new int[]{%s}".formatted( | ||
| CodeGenerationDataNameType.ints().con() | ||
| ); | ||
| } | ||
|
|
||
| @Override | ||
| public String name() { | ||
| return "int[]"; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isSubtypeOf(DataName.Type other) { | ||
| return other instanceof IntArrayType; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return name(); | ||
| } | ||
| } |
There was a problem hiding this comment.
We may want to have some sort of array type in the mainline library as well.
| static TemplateToken hashField(FieldConstant field) { | ||
| return Template.make("FIELD", (FieldConstant f) -> body( | ||
| let("BOXED", f.type.boxedTypeName()), | ||
| let("FIELD_NAME", f.name()), | ||
| """ | ||
| #BOXED.hashCode(#FIELD_NAME) + | ||
| """ | ||
| )).asToken(field); | ||
| } |
There was a problem hiding this comment.
Looks like you don't actually need any arguments in the Template here. You are already capturing field in the lambda.
| """ | ||
| 0; | ||
| } |
There was a problem hiding this comment.
You should probably rather joint a list of strings with " + ", rather than letting the hashField add the + that may feel a little lost there 😅
| final List<FieldConstant> fields = fieldTypes.stream() | ||
| .map(fieldType -> FieldConstant.of(fieldId.getAndIncrement(), fieldType)) | ||
| .toList(); |
There was a problem hiding this comment.
Why do you need the constants associated with the fields here?
There was a problem hiding this comment.
And what is the id good for?
| record FieldConstant(Object value, int id, PrimitiveType type) { | ||
| String name() { | ||
| return "v" + id; | ||
| } |
There was a problem hiding this comment.
What is the purpose of the id here? Do you ever need it, or just the name()?
| static TemplateToken field(FieldConstant field) { | ||
| return Template.make("FIELD", (FieldConstant f) -> body( | ||
| let("FIELD_TYPE", f.type), | ||
| let("FIELD_VALUE", f.value), |
There was a problem hiding this comment.
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.
| """ | ||
| static value class $Box { | ||
| """, | ||
| fields(fields), |
There was a problem hiding this comment.
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?
|
And it would be nice to see if we cannot use |
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 68e4a60 so that Boolean types are supported incheckEQ.This test has surfaced 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.
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_layoutand the way_nullable_layout_size_in_bytesis then used to decide whether to flatten or not. I've been usingPrintFieldLayoutoption 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.Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1522/head:pull/1522$ git checkout pull/1522Update a local copy of the PR:
$ git checkout pull/1522$ git pull https://git.openjdk.org/valhalla.git pull/1522/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1522View PR using the GUI difftool:
$ git pr show -t 1522Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1522.diff
Using Webrev
Link to Webrev Comment