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

[Vladimir Ivanov]

On Fri, 28 Oct 2022 15:35:38 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 one additional commit since the last revision:
> 
>   build fix

Thanks, Roland! Overall, looks very good.

Submitted hs-tier1 - hs-tier4 testing. (Earlier, it went through hs-tier1 - hs-tier8 without new failures.)

Some minor comments/suggestions follow.

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

> 733:   GrowableArray<ciInstanceKlass*>* result = NULL;
> 734:   GUARDED_VM_ENTRY(
> 735:     InstanceKlass* ik = get_instanceKlass();

Does it make sense to cache the result on `ciInstanceKlass` instance?

src/hotspot/share/ci/ciObjectFactory.cpp line 160:

> 158:     InstanceKlass* ik = vmClasses::name();                           \
> 159:     ciEnv::_##name = get_metadata(ik)->as_instance_klass();          \
> 160:     Array<InstanceKlass*>* interfaces = ik->transitive_interfaces(); \

What's the purpose of interface-related part of the code?

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

> 570: 
> 571:   TypeAryPtr::_array_interfaces = new TypePtr::InterfaceSet();
> 572:   GrowableArray<ciInstanceKlass*>* array_interfaces = ciArrayKlass::interfaces();

Maybe move the code into a constructor or a factory method?
After that, the only user of `TypePtr::InterfaceSet::add()` will be `TypePtr::interfaces()`.
It would be nice to make `TypePtr::InterfaceSet` immutable and cache query results (`InterfaceSet::is_loaded() ` and `InterfaceSet::exact_klass()`).

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

> 4838:       }
> 4839:       interfaces = this_interfaces.intersection_with(tp_interfaces);
> 4840:       return TypeInstPtr::make(ptr, ciEnv::current()->Object_klass(), interfaces, false, NULL,offset, instance_id, speculative, depth);

> NULL,offset

missing space

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

> 5735:       // below the centerline when the superclass is exact. We need to
> 5736:       // do the same here.
> 5737:       if (klass()->equals(ciEnv::current()->Object_klass()) && this_interfaces.intersection_with(tp_interfaces).eq(this_interfaces) && !klass_is_exact()) {

> this_interfaces.intersection_with(tp_interfaces).eq(this_interfaces)

Maybe a case for a helper method `InterfaceSet::contains(InterfaceSet)`?

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

> 5859:     bool klass_is_exact = ik->is_final();
> 5860:     if (!klass_is_exact &&
> 5861:         deps != NULL && UseUniqueSubclasses) {

Please, put `UseUniqueSubclasses` guard at the top of the method.

src/hotspot/share/opto/type.hpp line 1154:

> 1152:   // Respects UseUniqueSubclasses.
> 1153:   // If the klass is final, the resulting type will be exact.
> 1154:   static const TypeOopPtr* make_from_klass(ciKlass* klass, bool trust_interface = false) {

I'd suggest to use an enum (`trust_interfaces`/`ignore_interfaces`) instead of a `bool`, so the intention is clear at call sites.

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

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