RFR: 8314651: G1: Fix -Wconversion warnings in static fields of HeapRegion

[Thomas Schatzl]

On Mon, 21 Aug 2023 12:25:12 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> Use unsigned type for heap-region-size related shifts in HeapRegion.
>  
> Test: tier1-3

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/c1/g1BarrierSetC1.cpp line 161:

> 159:     __ move(xor_res, xor_shift_res);
> 160:     __ unsigned_shift_right(xor_shift_res,
> 161:                             LIR_OprFact::intConst(static_cast<jint>(HeapRegion::LogOfHRGrainBytes)),

One could maybe argue that this should be a `checked_cast`, but given that the value bounds are known there is no point...

src/hotspot/share/gc/g1/g1Arguments.cpp line 137:

> 135:   uint region_size_log_mb = HeapRegion::LogOfHRGrainBytes > LOG_M
> 136:                           ? HeapRegion::LogOfHRGrainBytes - LOG_M
> 137:                           : 0;

I would just assert that `HeapRegion::LogOfHRGrainBytes >= LOG_M` here. People messing with with minimum region size would need to check this code anyway.

Further, `LOG_M` should probably be calculated as `log2_exact(HeapRegion::min_size())` instead of using the constant directly (and renamed appropriately, something like `Log[Of]MinHeapRegionSize/GrainBytes`).

Then there would be no need to do the maximum calculation (my original suggestion would have been `MAX2(HeapRegion::LogOfHRGrainBytes - LOG_M, 0u)` btw)

src/hotspot/share/gc/g1/g1Arguments.cpp line 140:

> 138: 
> 139:   if (FLAG_IS_DEFAULT(G1RemSetArrayOfCardsEntries)) {
> 140:     uint max_cards_in_inline_ptr = G1CardSetConfiguration::max_cards_in_inline_ptr(HeapRegion::LogCardsPerRegion);

Not sure how this change has anything to do with -Wconversion, but okay :)

src/hotspot/share/gc/g1/g1CardTable.inline.hpp line 34:

> 32: inline uint G1CardTable::region_idx_for(CardValue* p) {
> 33:   size_t const card_idx = pointer_delta(p, _byte_map, sizeof(CardValue));
> 34:   return (uint)(card_idx >> HeapRegion::LogCardsPerRegion);

Not sure how this change has anything to do with -Wconversion but okay :)

src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 820:

> 818: 
> 819: #define VM_STRUCTS_JVMCI_G1GC(nonstatic_field, static_field) \
> 820:   static_field(HeapRegion, LogOfHRGrainBytes, uint32_t)

Suggestion:

  static_field(HeapRegion, LogOfHRGrainBytes, uint)


Seems to compile just fine here.

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

PR Review: https://git.openjdk.org/jdk/pull/15360#pullrequestreview-1599784043
PR Review Comment: https://git.openjdk.org/jdk/pull/15360#discussion_r1308457638
PR Review Comment: https://git.openjdk.org/jdk/pull/15360#discussion_r1308395646
PR Review Comment: https://git.openjdk.org/jdk/pull/15360#discussion_r1308396907
PR Review Comment: https://git.openjdk.org/jdk/pull/15360#discussion_r1308396990
PR Review Comment: https://git.openjdk.org/jdk/pull/15360#discussion_r1308423061