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

[Roland Westrelin]

Hi Vladimir,

Thanks for looking at this.

> 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?

It’s arbitrary so if you think 100 is too much, sure we can go with 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 reason I’d like PhaseTransform:: is_dominator() is that for 2 other prototypes I’m working on, I also had 2 copies of the same logic, one that I want apply during IGVN and one that I want apply during loop opts. I’d like to be able to write that logic only once in a clean way and be able to call it both from IGVN and loop opts. 

> 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.

Ok.

> 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.

In final_graph_reshaping we collect all control the memory node depends on and the one we keep is picked in gcm so can it really be incorrect to collect more controls?
The reason I made that change is that in the test case, the graph before the Phi is removed is:

LoadI -> CastPP 1 (to non NULL) -> Phi

after the Phi is removed, we create the new CastPP:

LoadI -> CastPP 1 (to non NULL) -> CastPP 2 (carries dependency)

CastPP 1 has a control that dominates CastPP 2, so in graph_final_reshaping we want to collect the control of both CastPPs.

(Actually, with the new Ideal transforms, the CastPPs will simplify but it’s not guaranteed in every cases)

> 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.

In IdealLoopTree::beautify_loops(), as I understand:

  if (_head->req() > 3 && !_irreducible) {
    split_outer_loop( phase );
    result = true;

  } else if (!_head->is_Loop() && !_irreducible) {
    // Make a new LoopNode to replace the old loop head                                                                                                                                                                                                                         
    Node *l = new LoopNode( _head->in(1), _head->in(2) );

don’t we skip irreducible loops when we create LoopNodes?

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

The reason I added that option is because the test doesn’t fail with the default GC (G1). The reason I think is that the G1 post barrier has a wide barrier that prevents the load of saved_not_null to be optimized out (that’s https://bugs.openjdk.java.net/browse/JDK-8087341).

Roland.

> 
> 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.
>>