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

[Roland Westrelin]

Hi Vladimir,

>>> 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.
> 
> What is reasonable number, you think, based on tests you have? I think 100 is waste of time but 10 could be not enough.

I don’t have evidence that 100 is justified so let’s go with 10 and increase it later on if we find it’s not sufficient. All performance testing I’ve performed was with 100. Should I redo perf testing?

>>> 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.
> 
> Understood. Does all these cases check only Cast nodes or others too? It may not safe in general case. Also since you are look in not on whole grpaph the method name should be something like is_near_dominator().

These cases are with different nodes, not only cast nodes.
If it’s called is_near_dominator(), then it can’t be a method shared by both loop opts and gvn so a logic that is applied during loop opts and gvn can't be written only once.


>>> 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).
> 
> Add @requires vm.gc=="Serial"
> 
> See:
> https://bugs.openjdk.java.net/browse/JDK-8062537

Ok.

Here is a new webrev:

http://cr.openjdk.java.net/~roland/8139771/webrev.01/

As you suggested I made CheckCastPP inherit from ConstraintCast. I also hit the following bug: one iteration of a loop is peeled which causes a CastPP to be pinned between the loop and the predicates. When a predicate that depends on the CastPP is moved out of the loop, it is moved above the CastPP. I fixed by marking all nodes that depend on a node pinned between a loop and the predicates as non loop invariant. I don’t think fixing it by moving the cast up above the predicates is a safe fix in general.

Roland.