[10] RFR(S): 8188785: CCP sets invalid type for java mirror load

[Vladimir Kozlov]

Hi Tobias

On 10/23/17 1:04 AM, Tobias Hartmann wrote:
> Hi Vladimir,
> 
> thanks for the review!
> 
> On 20.10.2017 18:43, Vladimir Kozlov wrote:
>> On 10/20/17 9:36 AM, Vladimir Kozlov wrote:
>>> Hmm. Is this only LoadP or general problem?
> 
> This is a general problem with nodes that compute their type not based 
> on immediate inputs.

I think we need to file a bug or rfe to fix other cases too.

> 
>>> May be add code to next lines when m->is_AddP() :
>>>
>>> 1734         if (m->bottom_type() != type(m)) { // If not already 
>>> bottomed out
>>> 1735           worklist.push(m);     // Propagate change to user
> 
> Where should I add that code exactly? My fix already checks for "ut != 
> type(u)".

My bad - I forgot that raw LoadP may not change its type but you still 
want to push its users when n (AddP) change its type.

> 
>>> I think we should do similar to PhaseIterGVN::add_users_to_worklist().
>>
>> Hmm, PhaseIterGVN::add_users_to_worklist() is not good example - it 
>> only puts near loads/stores. Should we fix it too?
> 
> Yes, I think it makes sense to update add_users_to_worklist() as well:
> http://cr.openjdk.java.net/~thartmann/8188785/webrev.01/

In add_users_to_worklist() you don't need to check type ut !=
 > type(u) - just push node on worklist.

Also can you remove ut->isa_instptr() check in *both* cases. And use 
u->is_Mem() instead of u->Opcode() == Op_LoadP to cover stores too.
The motivation is that original LoadP is raw as result memory operations 
which use it may look for more precise type of the field somewhere so 
they should be on worklist.

> 
>> Do we have other cases when we calculate type based not on immediate 
>> inputs but their inputs?
> 
> Yes, see code right above my changes:
>    // CmpU nodes can get their type information from two nodes up in the
>    // graph (instead of from the nodes immediately above). Make sure they
>    // are added to the worklist if nodes they depend on are updated, since
>    // they could be missed and get wrong types otherwise.
> 
> http://hg.openjdk.java.net/jdk10/hs/file/6126617b8508/src/hotspot/share/opto/phaseX.cpp#l1738 
> 

Okay.

Thanks,
Vladimir

> 
> The same goes for CallNodes and counted loop exit conditions (see 
> surrounding code).
> 
> I'm not aware of any other cases.
> 
> Thanks,
> Tobias
> 
>>> On 10/20/17 1:04 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> please review the following patch:
>>>> https://bugs.openjdk.java.net/browse/JDK-8188785
>>>> http://cr.openjdk.java.net/~thartmann/8188785/webrev.00/
>>>>
>>>> Since 8186777 [1], we require two loads to retrieve the java mirror 
>>>> from a klass oop:
>>>>
>>>> LoadP(LoadP(AddP(klass_oop, java_mirror_offset)))
>>>>
>>>> The problem is that now the type of the outermost LoadP does not 
>>>> depend on the inner LoadP (which has a raw pointer type) but on the 
>>>> type of the AddP which is one level up. CPP only propagates the 
>>>> types downwards to the direct users and as a result, the mirror 
>>>> LoadP ends up with an incorrect (too narrow/optimistic) type.
>>>>
>>>> I've verified the fix with the failing test and also verified that 
>>>> 8188835 [2] is a duplicate.
>>>>
>>>> Gory details:
>>>> During CCP, we compute the type of a Phi that merges oops of type A 
>>>> and B where B is a subtype of A. Since the type of the A input was 
>>>> not computed yet (it was initialized to TOP at the beginning of 
>>>> CCP), the Phi temporarily ends up with type B (i.e. with a type that 
>>>> is too narrow/optimistic). This type is propagated downwards and is 
>>>> being used to optimize a java mirror load from the klass oop:
>>>>
>>>> LoadP(LoadP(AddP(DecodeNKlass(LoadNKlass(AddP(CastPP(Phi)))))))
>>>>
>>>> The mirror load is then folded to TypeInstPtr::make(B) which is not 
>>>> correct because the oop can be of type A at runtime.
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8186777
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8188835