8318157: RISC-V: implement ensureMaterializedForStackWalk intrinsic

[omikhaltsova]

Please, review addition of the ensureMaterializedForStackWalk C2 intrinsic for RISC-V.
It's implemented by analogy to AArch64 and x86_64 [1].

The benchmark shows the following performance improvement on the T-Head RVB-ICE board:
Before

Benchmark                                             Mode  Cnt    Score    Error  Units
ScopedValues.CreateBindThenGetThenRemove_ScopedValue  avgt   10  873.072  12.051  ns/op
ScopedValues.bindThenGetNoRemove_ThreadLocal          avgt   10   55.507  18.006  ns/op
ScopedValues.bindThenGetThenRemove_ScopedValue        avgt   10  758.185  14.585  ns/op
ScopedValues.bindThenGetThenRemove_ThreadLocal        avgt   10  922.742   9.597  ns/op
ScopedValues.bindViaGet_ScopedValue                   avgt   10  603.164  13.694  ns/op
ScopedValues.bind_ScopedValue                         avgt   10  557.377   9.591  ns/op
ScopedValues.bind_ThreadLocal                         avgt   10  854.636  17.014  ns/op
ScopedValues.counter_ScopedValue                      avgt   10   35.367   0.421  ns/op
ScopedValues.counter_ThreadLocal                      avgt   10   45.345   0.283  ns/op
ScopedValues.setNoRemove_ScopedValue                  avgt   10   39.838   0.432  ns/op
ScopedValues.setNoRemove_ThreadLocal                  avgt   10   44.769   0.404  ns/op
ScopedValues.sixValues_ScopedValue                    avgt   10    2.566   0.006  us/op
ScopedValues.sixValues_ThreadLocal                    avgt   10    9.067   0.020  us/op
ScopedValues.thousandAdds_ScopedValue                 avgt   10    0.158   0.004  us/op
ScopedValues.thousandAdds_ThreadLocal                 avgt   10    9.662   0.019  us/op
ScopedValues.thousandIsBoundQueries                   avgt   10   31.203   0.451  ns/op
ScopedValues.thousandMaybeGets                        avgt   10  156.356   4.469  ns/op

After

Benchmark                                             Mode  Cnt    Score    Error  Units
ScopedValues.CreateBindThenGetThenRemove_ScopedValue  avgt   10  367.616   4.382  ns/op
ScopedValues.bindThenGetNoRemove_ThreadLocal          avgt   10   55.935  18.048  ns/op
ScopedValues.bindThenGetThenRemove_ScopedValue        avgt   10  371.733   2.835  ns/op
ScopedValues.bindThenGetThenRemove_ThreadLocal        avgt   10  902.112  12.709  ns/op
ScopedValues.bindViaGet_ScopedValue                   avgt   10  158.404   2.920  ns/op
ScopedValues.bind_ScopedValue                         avgt   10  158.365   2.328  ns/op
ScopedValues.bind_ThreadLocal                         avgt   10  908.319   9.238  ns/op
ScopedValues.counter_ScopedValue                      avgt   10   35.438   0.470  ns/op
ScopedValues.counter_ThreadLocal                      avgt   10   43.705   0.435  ns/op
ScopedValues.setNoRemove_ScopedValue                  avgt   10   39.848   0.638  ns/op
ScopedValues.setNoRemove_ThreadLocal                  avgt   10   47.336  11.987  ns/op
ScopedValues.sixValues_ScopedValue                    avgt   10    2.847   0.008  us/op
ScopedValues.sixValues_ThreadLocal                    avgt   10    9.066   0.017  us/op
ScopedValues.thousandAdds_ScopedValue                 avgt   10    0.160   0.005  us/op
ScopedValues.thousandAdds_ThreadLocal                 avgt   10    9.651   0.020  us/op
ScopedValues.thousandIsBoundQueries                   avgt   10   31.145   0.512  ns/op
ScopedValues.thousandMaybeGets                        avgt   10  160.829   6.699  ns/op

[1] JDK-8286666, 221e1a4#diff-018aa61d1a7aafcf70a535fcd40a318a4bd6511fd40ac39ce4be90cc52216749R3635

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8318157: RISC-V: implement ensureMaterializedForStackWalk intrinsic (Enhancement - P4)

Reviewers

• Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16808/head:pull/16808
$ git checkout pull/16808

Update a local copy of the PR:
$ git checkout pull/16808
$ git pull https://git.openjdk.org/jdk.git pull/16808/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16808

View PR using the GUI difftool:
$ git pr show -t 16808

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16808.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back omikhaltcova! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@omikhaltsova The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (98f7d263)

[RealFYang]

Hi, thanks for finding this. Looks OK. Any correctness test performed?

[openjdk]

@omikhaltsova This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8318157: RISC-V: implement ensureMaterializedForStackWalk intrinsic

Reviewed-by: fyang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 230 new commits pushed to the master branch:

• 940f67c: 8318854: [macos14] Running any AWT app prints Secure coding warning
• c864317: 8320888: Shenandoah: Enable ShenandoahVerifyOptoBarriers in debug builds
• d1e73b1: 8318626: GetClassFields does not filter out ConstantPool.constantPoolOop field
• ea6e92e: 8320945: problemlist tests failing on latest Windows 11 update
• 2584bf8: 8210410: Refactor java.util.Currency:i18n shell tests to plain java tests
• 454b116: 8320942: Only set openjdk-target when cross compiling linux-aarch64
• eb44baf: 8320937: support latest VS2022 MSC_VER in abstract_vm_version.cpp
• 62418c6: 8319444: Unhelpful failure output in TestLegalNotices
• cdd1a6e: 8313816: Accessing jmethodID might lead to spurious crashes
• b65ccff: 8320877: Shenandoah: Remove ShenandoahUnloadClassesFrequency support
• ... and 220 more: https://git.openjdk.org/jdk/compare/1802cb566e956febebc181da26a666bea4942e87...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealFYang) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[omikhaltsova]

@RealFYang, yes, tier1 tests passed.

The only thing, I'm hesitating about, is the size of __ nop(). I assume that it'll be 4 bytes because, for example, I see in code related to deoptimization the following:
src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp#L1290

    // First instruction must be a nop as it may need to be patched on deoptimisation
    {
      Assembler::IncompressibleRegion ir(masm);  // keep the nop as 4 bytes for patching.
      MacroAssembler::assert_alignment(__ pc());
      __ nop();  // 4 bytes
    } 

But from the other hand, according to the comments, in case of RVC __ nop() shall be 2 bytes:
src/hotspot/cpu/riscv/riscv.ad#L1292

    Assembler::CompressibleRegion cr(&_masm); // nops shall be 2-byte under RVC for alignment purposes.
    for (int i = 0; i < _count; i++) {
      __ nop();
    }

Is there possibility of error?

[RealFYang]

@RealFYang, yes, tier1 tests passed.

The only thing, I'm hesitating about, is the size of __ nop(). I assume that it'll be 4 bytes because, for example, I see in code related to deoptimization the following: src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp#L1290

    // First instruction must be a nop as it may need to be patched on deoptimisation
    {
      Assembler::IncompressibleRegion ir(masm);  // keep the nop as 4 bytes for patching.
      MacroAssembler::assert_alignment(__ pc());
      __ nop();  // 4 bytes
    } 

But from the other hand, according to the comments, in case of RVC __ nop() shall be 2 bytes: src/hotspot/cpu/riscv/riscv.ad#L1292

    Assembler::CompressibleRegion cr(&_masm); // nops shall be 2-byte under RVC for alignment purposes.
    for (int i = 0; i < _count; i++) {
      __ nop();
    }

Is there possibility of error?

Note that there is a Assembler::IncompressibleRegion ir(&_masm); declaration on the function entry [1]. This effectivly ensures that the nop planted here won't be compressed and thus will be 4-bytes in size.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/riscv.ad#L2369

[omikhaltsova]

@RealFYang thanks a lot for the explanation and reviewing!

/integrate

[openjdk]

@omikhaltsova
Your change (at version 98f7d26) is now ready to be sponsored by a Committer.

[VladimirKempik]

/sponsor

[openjdk]

Going to push as commit d6b4aa0.
Since your change was applied there have been 231 commits pushed to the master branch:

• 7766785: 8319372: C2 compilation fails with "Bad immediate dominator info"
• 940f67c: 8318854: [macos14] Running any AWT app prints Secure coding warning
• c864317: 8320888: Shenandoah: Enable ShenandoahVerifyOptoBarriers in debug builds
• d1e73b1: 8318626: GetClassFields does not filter out ConstantPool.constantPoolOop field
• ea6e92e: 8320945: problemlist tests failing on latest Windows 11 update
• 2584bf8: 8210410: Refactor java.util.Currency:i18n shell tests to plain java tests
• 454b116: 8320942: Only set openjdk-target when cross compiling linux-aarch64
• eb44baf: 8320937: support latest VS2022 MSC_VER in abstract_vm_version.cpp
• 62418c6: 8319444: Unhelpful failure output in TestLegalNotices
• cdd1a6e: 8313816: Accessing jmethodID might lead to spurious crashes
• ... and 221 more: https://git.openjdk.org/jdk/compare/1802cb566e956febebc181da26a666bea4942e87...master

Your commit was automatically rebased without conflicts.

[openjdk]

@VladimirKempik @omikhaltsova Pushed as commit d6b4aa0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.