RFR: 8296448: RISC-V: Fix temp usages of heapbase register killed by MacroAssembler::en/decode_klass_not_null
Fei Yang
fyang at openjdk.org
Tue Nov 8 05:47:33 UTC 2022
On Mon, 7 Nov 2022 04:03:57 GMT, Xiaolin Zheng <xlinzheng at openjdk.org> wrote:
> Please see the JBS issue for more crash details.
>
> To reproduce using a cross-compiled build:
>
> # dump one cds-nocoops.jsa
> <java> -XX:-UseCompressedOops -XX:+UseCompressedClassPointers -Xshare:dump -Xlog:cds* -version
>
> # reproduce
> <java> -XX:-UseCompressedOops -XX:+UseCompressedClassPointers -Xshare:on -XX:-TieredCompilation \
> -Xlog:cds* -Xlog:gc+metaspace=info -jar renaissance-gpl-0.14.1.jar -r 1 movie-lens
>
>
> `MacroAssembler::en/decode_klass_not_null` uses the heapbase register as a temp register in the interpreter, which may kill the in-use value when enabling C2 compilation and `UseCompressedClassPointers` meanwhile disabling `UseCompressedOops`. C1 won't have this issue for the xheapbase is not its allocation candidate. When CDS is enabled, the narrow klass base is mapped to some address like `0x0000000800000000`, so `MacroAssembler::decode_klass_not_null`, which lacks registers, will use `xheapbase` as a temp to load the klass base and kill the register in the interpreter. So adding a `-XX:+DeoptimizeALot` can speedily reproduce the issue.
>
> To solve this, we shall decouple the xheapbase used as a temp register in `MacroAssembler::en/decode_klass_not_null`. AArch64 has advanced instructions so one register is enough to handle the logic. But in RISC-V we require at least two.
>
> This patch introduces another argument `tmp` to related functions to decouple and eliminate such heapbase usages. One thing that deserves noticing is the `cmp_klass` case, which usually gets used at the beginning of a method entry when `t1` is used as ic holder klass and `t0` is occupied there. These positions are special since nearly all registers are usable except ones used for arguments and special purposes (thread register, etc.). I propose to use a call-clobbered `t2` register here, to keep aligning the `i2c2i_adapter` logic[1].
>
> Tested hotspot tier1~4 on QEMU; jdk tier1~tier2 and hotspot tier1~tier2 on my Hifive unmatched board, and the reproducible movie-lens benchmark.
>
> Thanks,
> Xiaolin
>
> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp#L629
Thanks for fixing this. Several minor nits.
src/hotspot/cpu/riscv/methodHandles_riscv.cpp line 81:
> 79: __ beqz(obj, L_bad);
> 80: __ push_reg(RegSet::of(temp, temp2), sp);
> 81: __ load_klass(temp, obj, temp2);
Could you please help rename 'temp' to 'temp1' at the same time?
src/hotspot/cpu/riscv/riscv.ad line 1741:
> 1739:
> 1740: Label skip;
> 1741: __ cmp_klass(j_rarg0, t1, t0, t2 /* as a tmp */, skip);
You might also want to add comment for 't0' here: /* as a temp */
src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp line 1409:
> 1407: const Register ic_reg = t1;
> 1408: const Register receiver = j_rarg0;
> 1409: const Register tmp_reg = t2;
No need to introduce 'tmp_reg' here. I think you can use 't2' directly like the change you made in file: src/hotspot/cpu/riscv/riscv.ad
-------------
PR: https://git.openjdk.org/jdk/pull/11010
More information about the hotspot-dev
mailing list