On Thu, 29 Aug 2024 22:03:39 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add in graal flags and a comment.
>
> src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp line 76:
>
>> 74: load_klass(tmp, Roop);
>> 75: z_lb(tmp, Address(tmp, Klass::misc_flags_offset()));
>> 76: testbit(tmp, exact_log2(KlassFlags::_misc_is_value_based_class));
>
> Suggestion:
>
> z_tm(Klass::misc_flags_offset(), tmp,
> KlassFlags::_misc_is_value_based_class);
>
> or
> Suggestion:
>
> z_tm(Address(tmp, Klass::misc_flags_offset()),
> KlassFlags::_misc_is_value_based_class);
Thank you for the corrections. I changed all of these in the s390 code.
> src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp line 62:
>
>> 60: load_klass(hdr, obj, rscratch1);
>> 61: movb(hdr, Address(hdr, Klass::misc_flags_offset()));
>> 62: testl(hdr, KlassFlags::_misc_is_value_based_class);
>
> Suggestion:
>
> testb(Address(hdr, Klass::misc_flags_offset()),
> KlassFlags::_misc_is_value_based_class);
I corrected these also in the x86 code. This is much better.
> src/hotspot/share/ci/ciKlass.cpp line 233:
>
>> 231: jint ciKlass::misc_flags() {
>> 232: assert(is_loaded(), "not loaded");
>> 233: GUARDED_VM_ENTRY(
>
> To Compiler folks: I don't think the VM_ENTRY is necessary, but if it is,
> then we should consider entering VM mode once and caching/memoizing these
> immutable flag values in the ciKlass.
I added a global typedef klass_flags_t because it didn't look confusing vs
KlassFlags and KlassFlags_t, and the lower case convention is something we
usually use for typedefs.
> src/hotspot/share/classfile/classFileParser.cpp line 5176:
>
>> 5174:
>> ik->set_declares_nonstatic_concrete_methods(_declares_nonstatic_concrete_methods);
>> 5175:
>> 5176: assert(!_is_hidden || ik->is_hidden(), "must be set already");
>
> Is this an optimization independent of the current change?
No, it's needed by the current change because with these Flags.hpp
implementations, we assert that they are only assigned once i.e. const after
classfile parsing. This was a second assignment.
> src/hotspot/share/oops/klassFlags.hpp line 56:
>
>> 54: // These flags are write-once before the class is published and then
>> read-only
>> 55: // so don't require atomic updates.
>> 56: u1 _flags;
>
> Suggestion:
>
> typedef u1 KlassFlags_t;
> KlassFlags_t _flags;
>
> Can we have a typedef so C++ code that doesn't care about the size doesn't
> need to change if we later make it u2 or u4?
Adding it here makes us refer to it as KlassFlags::KlassFlags_t which is pretty
noisy.
> src/hotspot/share/opto/library_call.cpp line 3774:
>
>> 3772:
>> 3773: // Use this for testing if Klass is_hidden, has_finalizer, and
>> is_cloneable_fast.
>> 3774: Node* LibraryCallKit::generate_misc_flags_guard(Node* kls, int
>> modifier_mask, int modifier_bits, RegionNode* region) {
>
> It looks like we could refactor generate_misc_flags_guard and
> generate_access_flags_guard with a common generate_klass_accessor_guard that
> takes the offset and type as parameters.
Yes, copying this code was a bit painful but it was hard to get right. I
refactored the common code into generate_mods_flags_guard, so we pass in the
modifier node and eliminate the duplicate code.
> src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java
> line 378:
>
>> 376: HotSpotVMConfig config = config();
>> 377: int miscFlags = UNSAFE.getByte(getKlassPointer() +
>> config.klassMiscFlagsOffset);
>> 378: return (miscFlags & config().jvmAccHasFinalizer) != 0;
>
> Suggestion:
>
> return (miscFlags & config.jvmAccHasFinalizer) != 0;
Also removed local config variable.
> src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java
> line 1118:
>
>> 1116: int miscFlags = UNSAFE.getByte(getKlassPointer() +
>> config.klassMiscFlagsOffset);
>> 1117: return (miscFlags & config().jvmAccIsCloneableFast) != 0;
>> 1118: }
>
> Maybe introduce getMiscFlags() helper like existing getAccessFlags()?
Seems not useful for these two references.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738609365
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738610399
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738611867
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738642433
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738644664
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738647156
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738688415
PR Review Comment: https://git.openjdk.org/jdk/pull/20719#discussion_r1738645913
