RFR(S): 8139771: Eliminating CastPP nodes at Phis when they all come from a unique input may cause crash

[Vladimir Kozlov]

Thank you, Roland, for sending performance numbers. They are good - no 
regression.

I was confused by naming first and by placement of code.
And mixing code for casts and code in gcm. In reality the cast code 
tries to find only "immediate"/near dominating cast. So why (i >= 100) 
and not, lets say, >= 10?

I am not sure that code should be in Phase classes. It is only work for 
cast nodes. I think it should be in ConstraintCast.
The only thing you use is to set linear_only parameter. I know that in 
Identity we don't have a way to find is it GVN or IGVN. It is old 
problem we should fix. For example instead of passing can_reshape to 
Ideal it could be field in PhaseTransform set by constructor. Then you 
could access it from Identity and Ideal.

An other thing is CheckCastPP. It should be subclass of 
ConstraintCastNode I think. Then make_cast() and dominating_cast() could 
be methods of ConstraintCastNode.

Gcm changes are fine to me.

I did not get how final_graph_reshaping change related to these changes. 
We try to assign control to memory which is dominated by current CastPP 
with control. So if you have chain of CastPP with control you will 
assign control of first (most dominating) and not the last which is near 
mem node. I don't think it is correct.

I am worry a little about irreducible loops which have several entries. 
So you may get into a trouble by checking ordinary loop in 
IfNode::up_one_dom(). May be only counted loops? Or check the presence 
of irreducible loops.

Remove -XX:+UseSerialGC flag from test. Otherwise we will get error when 
testing will try to use other GC.

Thanks,
Vladimir

On 12/4/15 5:08 AM, Roland Westrelin wrote:
> http://cr.openjdk.java.net/~roland/8139771/webrev.00/
>
> In PhiNode::Ideal(), when all uncasted inputs are identical, the Phi is removed which can cause a control dependency to be lost. See the test case (which includes a step by step description of how this can lead to a bad graph) for an example. The test case crashes on sparc with -XX:+StressGCM.
>
> To fix that I propose that rather than simply replacing the Phi by its uncasted input, we replace it by a CastPP that is specially marked as carrying a dependency (similar to what we have for CastII). That fixes this issue and should protect us from other similar issues:
>
> - I moved the _carry_dependency from CastII to ConstraintCast and CheckCastPP so it applies to CastPP, CastII and CheckCastPP
> - I added code to remove the casts that carry a dependency if there’s a dominating cast with identical inputs and a more restrictive type. I made is_dominator() a virtual method of PhaseTransform so we can have an implementation in GVN and use the same code during GVN and loop opts
> - We can now have a chain of CastPP with a control so the code in final_graph_reshaping shouldn’t go follow through a CastPP only if its control is null
> - I changed the code in PhaseCFG::schedule_pinned_nodes() to handle controls that are in the same block
>
> I performed extensive perf testing on x86 and sparc and found not statistically significant regressions.
>
> Roland.
>