8372700: [lworld] compiler/c2/irTests/stable/* fail with --enable-preview

[marc-chevalier]

Flat accesses to a stable value can be expanded in a non-atomic way if the stable field is already initialized since they are read-only at this point. That allows to make more optimizations, and in particular to replace the access by a constant if it's known at compilation time.

There are 2 cases. First, flat stable non-array field. In this case, the value is known to be stable if the value is non-null, that is if the null-marker of the said field is 1. If we can find that the null marker has a constant value that is non zero, we expand non-atomically. That is done by finding the field we are trying to get from the offset. From the field, we can find the offset of the null marker, and then the null marker ciField, allowing to fetch its value if the holder is known to be a constant oop.

The other case is stable flat array. In this case, we need to find index of the containing element of the array, then with the offset, we can find the field we are trying to get. Finding the null marker here is a bit more tricky. Let's say we have

value class MyValue {
    int x;
}
class C {
    MyValue v;  // assumed flat.
}

the null marker for v is somewhat a field of C, as well as v.x. So I can just use field_value to get the null marker. But in MyValue[], there isn't a single field for the null marker, but one "field" for each cell of the array, and there isn't a nice containing type in which it lives. The only way to get each piece of the array is by index (or offset). So, I needed specialized functions to access the null marker of a cell given the index/offset.

I also had to implement of bit of accessors for ciFlatArray. First, implement element_value_by_offset in ciFlatArray since the implementation of ciArray (super-class) was used, which computes the index from the provided offset, assuming a size of elements that doesn't take flattening into account as it used only the basic type, and not the layout helper. But also helpers to get a field of the flattened value class in a cell, to allow constant folding to get the constant value in an array.

The last part of the puzzle, necessary to make constant folding happen (see Type::make_constant_from_field), is to say that a field of a flattened inline type field is constant if the containing field if constant. In the words of the previous example, that means x is constant in C if v is strict and final (already there), or if v is constant itself. That matches what we do in void ciField::initialize_from(fieldDescriptor* fd):

valhalla/src/hotspot/share/ci/ciField.cpp

Line 341 in 26b7640

_is_constant = is_stable_field || trust_final_non_static_fields(_holder);

Thanks,
Marc

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8372700: [lworld] compiler/c2/irTests/stable/* fail with --enable-preview (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1826/head:pull/1826
$ git checkout pull/1826

Update a local copy of the PR:
$ git checkout pull/1826
$ git pull https://git.openjdk.org/valhalla.git pull/1826/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1826

View PR using the GUI difftool:
$ git pr show -t 1826

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1826.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@marc-chevalier this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8372700
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

[mlbridge]

Webrevs

• 02: Full (b64ff8ce)
• 01: Full - Incremental (80564f4b)
• 00: Full (9269cf54)

[TobiHartmann]

I had a first quick look at this and left a few comments. Will do a full review after vacation.

I assume the folding should also work for arrays with different layouts, right? I think we need tests for all of them, i.e. all ValueClass.new*Array() variants.

[TobiHartmann]

Should this be an assert?

[marc-chevalier]

Matter of point of view. I prefer to have the semantics

if inlinetype => get the null marker
else return invalid constant (since it doesn't make sense).

rather than

check is inline type
get null marker

At call site, in the first case, I need to

nm = inst->null_marker_value();
nm is true? (might be false or invalid)

in the second, I need to do

is inst an inline type?
if so, nm = inst->null_marker_value();
nm is true? (might be false, but not invalid)

I prefer the first pattern, it's rather safer, and I think a lot of the code involving ciConstant have this behavior of "you get the constant if I can, otherwise, you get invalid".

[TobiHartmann]

This needs a comment explaining why barriers are sometimes observed.

[marc-chevalier]

I agree. The problem is that you told me it's expected, but I forgot the reason.

[TobiHartmann]

Do we really need a cache here?

[marc-chevalier]

Maybe? I read

valhalla/src/hotspot/share/ci/ciObject.hpp

Lines 62 to 63 in 69399ce

	// Cache constant value lookups to ensure that consistent values are observed during compilation.
	class ConstantValue {

and

valhalla/src/hotspot/share/ci/ciObject.cpp

Lines 173 to 175 in 69399ce

	// Cache constant value lookups to ensure that consistent values are observed
	// during compilation because fields may be (re-)initialized concurrently.
	ciConstant ciObject::check_constant_value_cache(int off, BasicType bt) {

It seems to be more a correctness thing than a performance matter. It couldn't see why I wouldn't have a similar risk.