This PR prevents creating a missed optimization opportunity that originates
from creating inconsistent `TypeArrayPtr` types.
## Context
The link between the root cause of this issue and how it gets reported as a
missing optimization opportunity is quite subtle.
We initially have a chain of two `CheckCastPP` nodes, where the first one is a
cast to `not null free` and the second one is a cast to `flat`:
...
CheckCastPP
|
CheckCastPP
...
Let's look at what happens in `ConstraintCastNode::Value`:
In the initial GVN pass of `ConstraintCastNode::Value` for the **second node**,
we have (only looking at `not_null_free`):
- `in_type`
- `not_null_free` is `true`
- `not_null_free` (speculative) is `false`
- `_type`
- `not_null_free` is `false`
- `not_null_free` (speculative, implicitly) is `false`
- `ft` (join of `in_type` and `_type`)
- `not_null_free` is `true`
- `not_null_free` (speculative) is `false`
Note that here the speculative part of `ft` is eventually dropped because in
`cleanup_speculative` we keep the speculative part only if it contains
information about flat-/nullability:
```c++
const Type* TypeAryPtr::cleanup_speculative() const {
if (speculative() == nullptr) {
return this;
}
// Keep speculative part if it contains information about flat-/nullability
const TypeAryPtr* spec_aryptr = speculative()->isa_aryptr();
if (spec_aryptr != nullptr && !above_centerline(spec_aryptr->ptr()) &&
(spec_aryptr->is_not_flat() || spec_aryptr->is_not_null_free())) {
return this;
}
return TypeOopPtr::cleanup_speculative();
}
In the IGVN verification, we have a second pass of `ConstraintCastNode::Value`:
- `in_type` (same as first pass)
- `not_null_free` is `true`
- `not_null_free` (speculative) is `false`
- `_type` (changed)
- `not_null_free` is `true`
- `not_null_free` (speculative, implicitly) is `true`
- `ft` (join of `in_type` and `_type`)
- `not_null_free` is `true`
- `not_null_free` (speculative) is `true`
This time the speculative type is kept, however, because it contains
information about not-nullability.
Now the second pass happens in IGVN verification, and we hit the missed
optimization opportunity assert because we are not expected to make progress
again (the input has not changed in the meantime).
## Root cause
Now we could arguably clean up the resulting speculative type as well as it is
redundant. But this would obfuscate the fact the initial type of the first
`CheckCastPP` is already wrong: the speculative type should contain at least as
much information as the non-speculative type. In this situation, however, the
non-speculative type has `not_null_free=true`, which is a stronger guarantee
than the speculative type (`not_null_free=false`).
This happens because the speculative type originates from observing that the
array is null restricted from profiling information. The type is then cast to
`not null free`in `TypeAryPtr::cast_to_not_null_free`. This is where things
start to go wrong: we only cast the non-speculative part, assuming that the
speculative part is empty and that it will be implicitly cast as well. But it
is not the case here, and the speculative type ends up "lagging behind" the
non-speculative part. This eventually leads to the situation explained in the
previous section.
The solution is to explicitly cast the speculative type to `not null free` as
well. To avoid hitting an inconsistency assert, we also need to make sure that
it is not `null free` (we can use `TypeAryPtr::cast_to_null_free(false)`). Now
I think this is actually only half-correct: if the speculative type is `null
free` and we cast it to `not null free`, we should obtain something like
`TopNullFree`, similarly to the existing `FlatInArray::TopFlat`. If the
reviewers agree with this analysis, I will file a new RFE to adress it, as it
may require a larger changeset.
### Testing
- [x] tier1-3, plus some internal testing
Thank you for reviewing!
-------------
Commit messages:
- Syntax in comment
- Spaces
- Comments
- Cleanup test
- Revert "8380395: [lworld] ProblemList 4 sub-tests of
compiler/valhalla/inlinetypes/TestCallingConvention.java"
- Remove stuff
- Format
- Fix, first version
- Add reduced test
Changes: https://git.openjdk.org/valhalla/pull/2242/files
Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2242&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8367624
Stats: 96 lines in 3 files changed: 91 ins; 4 del; 1 mod
Patch: https://git.openjdk.org/valhalla/pull/2242.diff
Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2242/head:pull/2242
PR: https://git.openjdk.org/valhalla/pull/2242
