This PR prevents a missed optimization caused by a breach of idempotence in
`InlineTypeNode::Value` when handling contradicting method return profiles.
## Conflicting return profiles
Suppose we have the following java code, and that we have record profile data
for both callsites (`call(...)` and `callWrapper(...)`):
static MyValue callWrapper(...) {
return call(...); // callsite #1
}
public static MyValue test() {
return callWrapper(...); // callsite #2
}
It could be that `test` is not the only place where we call `callWrapper(...)`,
and that the other call sites result in wildy different executions of callsite
`#1` with wildly different return values at that callsite.
Suppose that callsite `#1` always returns `null`, except when `callWrapper` is
called from callsite `#2` where it throws an exception. In
`ciMethod::return_profiled_type`, we end up with the following `ProfilePtrKind`:
- For callsite `#1`: `ProfileAlwaysNull`, because we have only observed `null`
(the rest of the time an exception was thrown, so we didn't see anything)
- For callsite `#2`: `ProfileNeverNull`, because we have never observed a
return value and this is basically the default
Please look at the test from this PR for a more complete example.
These two profiles contradict each other. If we do incremental inlining
(probably not limited to it, but easier to reproduce), we can easily end up in
a situation where a speculative type derived from the return of the callsite is
`NotNull` before inlining the outer callsite, and then moves to `ptr:null`
after inlining. This can confuse `Value` implementations that rely on
`filter_speculative`. This is handled gracefully for `ConstraintCastNode`, but
not for `InlineTypeNode`.
## Missed optimization
In `InlineTypeNode::Value`, we have:
https://github.com/openjdk/valhalla/blob/e391f76e7db834f50c1a0af466daf95a65a79f23/src/hotspot/share/opto/inlinetypenode.cpp#L1619
Suppose we have the following types (only the speculative part is shown):
oop:
instptr:compiler/valhalla/inlinetypes/TestPollutedCallsiteProfileInlineType$MyValue:NotNull:exact
*,iid=bot (flat in array)
_type:
instptr:compiler/valhalla/inlinetypes/TestPollutedCallsiteProfileInlineType$MyValue:NotNull:exact
*,iid=bot (flat in array)
After this step we have `t = oop = _type`.
The oop input suddenly changes types because of incremental inlining, as
explained in the previous section. It goes from `NotNull` to `ptr:null` because
the type from the polluted profile propagates.
Then at the next IGVN pass, we have:
oop: ptr:null
_type:
instptr:compiler/valhalla/inlinetypes/TestPollutedCallsiteProfileInlineType$MyValue:NotNull:exact
*,iid=bot (flat in array)
Intuitively, the resulting type should be `TOP`. But
`TypePtr::cleanup_speculative` does not like it, and we end up with
`t.speculatve() = nullptr`:
https://github.com/openjdk/valhalla/blob/e391f76e7db834f50c1a0af466daf95a65a79f23/src/hotspot/share/opto/type.cpp#L2966-L2979
We end up with no speculative type, which means `speculative =
non_speculative`. Then in verification we do another `Value` pass:
oop: ptr:null
_type: nullptr
This gives us `ptr:null` for the resulting speculative type. This breaks the
idempotence of `Value`, and is reported as missed optimization.
## Fix
I think ideally we would like to be able to have `TOP` as a valid speculative
type (this is the most accurate it could get), but this would clearly break a
lot of things (I have tried). I think for now we should have the same behavior
as in `ConstraintCastNode::Value`: detect that the speculative type was
suppressed by `cleanup_speculative`, and call `filter_speculative` again when
it is the case. This is roughly the same as
[JDK-8371716](https://bugs.openjdk.org/browse/JDK-8371716).
### Testing
This issue was initially as a failure in `test59` of
`TestCallingConvention.java` with `-XX:-TieredCompilation
-XX:VerifyIterativeGVN=1110`. I think this specific test exhibits some quite
interesting behavior, but is quite hard to debug (it's not the first time it is
failing). For this reason I really wanted to reduce this to a less opaque test
case, and I think this test pattern (especially the profiling part) could be
useful in the future as well.
- [x] tier1-3, plus some internal testing
Thank you for reviewing!
---------
- [x] I confirm that I make this contribution in accordance with the [OpenJDK
Interim AI Policy](https://openjdk.org/legal/ai).
-------------
Commit messages:
- More details
- Details
- Reintroduce test59 after commenting out in JDK-8367624
- Fix
- Add test
Changes: https://git.openjdk.org/valhalla/pull/2351/files
Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2351&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8380927
Stats: 102 lines in 3 files changed: 87 ins; 1 del; 14 mod
Patch: https://git.openjdk.org/valhalla/pull/2351.diff
Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2351/head:pull/2351
PR: https://git.openjdk.org/valhalla/pull/2351
