RFR: 8314651: G1: Fix -Wconversion warnings in static fields of HeapRegion
Thomas Schatzl
tschatzl at openjdk.org
Tue Aug 29 09:10:18 UTC 2023
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
More information about the graal-dev
mailing list