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

[Tobias Hartmann]

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.

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

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

> 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

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