RFR: 6312651: Compiler should only use verified interface types for optimization [v3]

[Vladimir Ivanov]

On Wed, 9 Nov 2022 14:47:41 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> This change is mostly the same I sent for review 3 years ago but was
>> never integrated:
>> 
>> https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2019-May/033803.html
>> 
>> The main difference is that, in the meantime, I submitted a couple of
>> refactoring changes extracted from the 2019 patch:
>> 
>> 8266550: C2: mirror TypeOopPtr/TypeInstPtr/TypeAryPtr with TypeKlassPtr/TypeInstKlassPtr/TypeAryKlassPtr
>> 8275201: C2: hide klass() accessor from TypeOopPtr and typeKlassPtr subclasses
>> 
>> As a result, the current patch is much smaller (but still not small).
>> 
>> The implementation is otherwise largely the same as in the 2019
>> patch. I tried to remove some of the code duplication between the
>> TypeOopPtr and TypeKlassPtr hierarchies by having some of the logic
>> shared in template methods. In the 2019 patch, interfaces were trusted
>> when types were constructed and I had added code to drop interfaces
>> from a type where they couldn't be trusted. This new patch proceeds
>> the other way around: interfaces are not trusted when a type is
>> constructed and code that uses the type must explicitly request that
>> they are included (this was suggested as an improvement by Vladimir
>> Ivanov I think).
>
> Roland Westrelin has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - review
>  - review
>  - review
>  - review
>  - review

Much better, thanks!

Minor comments/suggestions follow.

FTR test results are clean. I'll submit performance testing shortly.

src/hotspot/share/ci/ciArrayKlass.cpp line 108:

> 106: }
> 107: 
> 108: GrowableArray<ciInstanceKlass*>* ciArrayKlass::interfaces() {

FTR there's a subtle asymmetry between `ciArrayKlass::interfaces()` and `ciInstanceKlass::transitive_interfaces()` when it comes to memory allocation: the former allocates from resource area while the latter from compiler arena.

It doesn't cause any problems since `ciArrayKlass::interfaces()` is used only in `Type::Initialize_shared()` to instantiate shared `TypeAryPtr::_array_interfaces`, but it took me some time to find that out. 

Maybe a helper method in `type.cpp` is a better place for that logic.

src/hotspot/share/ci/ciInstanceKlass.cpp line 736:

> 734:   if (_transitive_interfaces == NULL) {
> 735:     GUARDED_VM_ENTRY(
> 736:             InstanceKlass* ik = get_instanceKlass();

A candidate for `compute_transitive_interfaces()` helper method?

src/hotspot/share/opto/subnode.cpp line 1050:

> 1048:   // return the ConP(Foo.klass)
> 1049:   assert(mirror_type->is_klass(), "mirror_type should represent a Klass*");
> 1050:   return phase->makecon(TypeKlassPtr::make(mirror_type->as_klass(),  Type::trust_interfaces));

Extra space.

src/hotspot/share/opto/type.cpp line 4003:

> 4001:     assert(loaded->ptr() != TypePtr::Null, "insanity check");
> 4002:     //
> 4003:     if(      loaded->ptr() == TypePtr::TopPTR ) { return unloaded; }

Missing space after `if`.

src/hotspot/share/opto/type.cpp line 4017:

> 4015:   // Both are unloaded, not the same class, not Object
> 4016:   // Or meet unloaded with a different loaded class, not java/lang/Object
> 4017:   if( ptr != TypePtr::BotPTR ) {

Missing space after `if`.

-------------

PR: https://git.openjdk.org/jdk/pull/10901