RFR: 8297933: [REDO] Compiler should only use verified interface types for optimization [v2]
Roland Westrelin
roland at openjdk.org
Wed Dec 14 13:50:47 UTC 2022
> This PR re-does 6312651 (Compiler should only use verified interface
> types for optimization) with a couple fixes I had pushed afterward
> (8297556 and 8297343) and fixes for some other issues.
>
> The trickiest one is a fix for 8297345 (C2: SIGSEGV in
> PhaseIdealLoop::push_pinned_nodes_thru_region) for which I added a
> test case. The crash occurs because a (If (Bool (CmpP (LoadKlass ..))))
> only has a single projection. It lost the other projection because of
> a CheckCastPP that becomes top. Initially the pattern is, in pseudo
> code,:
>
> if (obj.klass == some_class) {
> obj = CheckCastPP#1(obj);
> }
>
> obj itself is a CheckCastPP that's pinned at a dominating if. That
> dominating if goes through split through phi. The LoadKlass for the
> pseudo code above also has control set to the dominating if being
> transformed. This result in:
>
> if (phi1 == some_class) {
> obj = CheckCastPP#1(phi2);
> }
>
> with phi1 = (Phi (LoadKlass obj) (LoadKlass obj)) and phi2 = (Phi obj obj)
> with obj = (CheckCastPP#2 obj')
>
> PhiNode::Ideal() transforms phi2 into a new CheckCastPP:
> (CheckCastPP#3 obj' obj') with control set to the region right above
> the if in the pseudo code above. There happens to be another
> CheckCastPP at the same control which casts obj' to a narrower
> type. So the new CheckCastPP#3 is replaced by that one (because of
> ConstraintCastNode::dominating_cast())and pseudo code becomes:
>
> if (phi1 == some_class) {
> obj = CheckCastPP#1(CheckCastPP#4(obj'));
> }
>
> and then:
>
> if (phi1 == some_class) {
> obj = top;
> }
>
> because the types of the 2 CheckCastPPs conflict. That would be ok if:
>
> phi1 == some_class
>
> would constant fold. It would if the test was:
>
> if (CheckCastPP#4(obj').klass == some_klass) {
>
> but because of split if, the (CmpP (LoadKlass ..)) and the
> CheckCastPP#1 ended up with 2 different object inputs that then were
> transformed differently. The fix I propose is to have split if clone the entire:
>
> (Bool (CmpP (LoadKlass (AddP ..))))
>
> down the same way (Bool (CmpP ..)) is cloned down. After split if, the
> pseudo code becomes:
>
> if (phi.klass == some_class) {
> obj = CheckCastPP#1(phi);
> }
>
> The bug can't occur because the CheckCastPP and (CmpP (LoadKlass ..))
> operate on the same phi input. The change in split_if.cpp implements
> that.
>
> The other fixes are:
>
> - arraycopynode.cpp: a crash happens because dest_offset and
> src_offset are the same. The call to transform that results in
> src_scale, causes src_offset (and thus dest_offset) to become
> dead. The fix is to add a hook node to preserve dest_offset. This is
> unrelated to 6312651 but it triggers with that change for some
> reason.
>
> - castnode.cpp: I removed CheckCastPPNode::Identity(), a piece of code
> that the change in the handling of interfaces make obsolete and that
> I missed in the PR for 6312651.
>
> - castnode.cpp: the change in CheckCastPPNode::Value() fixes a rare
> assert when during CCP, Value() is called with an input raw constant
> ptr.
>
> - type.cpp: a _klass = NULL field in arrays used to indicate only top
> or bottom but I changed that so _klass is only guaranteed non null
> for basic type arrays. The fix in type.cpp updates a piece of code
> that I didn't adapt to the new meaning of _klass = NULL.
>
> - the other changes are due to StressReflectiveCode. With 6312651, a
> CheckCastPP can fold to top if it sees a type for its input that
> conflicts with its own type. That wasn't the case before. So if a
> type check fails, a CheckCastPP will fold to top and the control
> flow branch it's in must die. That doesn't always happen with
> StressReflectiveCode: the CheckCastPP folds but not the control flow
> path. With ExpandSubTypeCheckAtParseTime on, that's because of a
> code path in LoadNode::Value() that's disabled with
> StressReflectiveCode. With ExpandSubTypeCheckAtParseTime off, it's
> because Compile::static_subtype_check() is always pessimistic with
> StressReflectiveCode but it's used by SubTypeCheckNode::sub() to
> find when a node can constant fold.
Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
test fix
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/11666/files
- new: https://git.openjdk.org/jdk/pull/11666/files/05bc4b97..4b86eb58
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=11666&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=11666&range=00-01
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/11666.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11666/head:pull/11666
PR: https://git.openjdk.org/jdk/pull/11666
More information about the hotspot-compiler-dev
mailing list