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

[Vladimir Kozlov]

On 10/20/17 9:36 AM, Vladimir Kozlov wrote:
> Hmm. Is this only LoadP or general problem?
> 
> 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
> 
> 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?

Do we have other cases when we calculate type based not on immediate 
inputs but their inputs?

Thanks,
Vladimir

> 
> Thanks,
> Vladimir
> 
> 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