8358556: Assert when running with -XX:-UseLibmIntrinsic #25680
Conversation
…d runtime implementations
|
👋 Welcome back missa-prime! A progress list of the required criteria for merging this PR into |
|
@missa-prime This change is no longer ready for integration - check the PR body for details. |
|
@missa-prime The following label will be automatically applied to this pull request:
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. |
|
@dholmes-ora Does this fix the -XX:-InlineIntrinsics assertion failures when running CubeRootTests.java and HyperbolicTests.java? I couldn't reproduce those, but I could reproduce the one with |
Webrevs
|
| case vmIntrinsics::_dtanh: | ||
| case vmIntrinsics::_dcbrt: |
There was a problem hiding this comment.
You could also move _floatToFloat16 and _float16ToFloat to here from the switch below to make it consistent.
There was a problem hiding this comment.
Ok, I moved them.
…rs could be used later on
| @@ -316,6 +314,10 @@ bool vmIntrinsics::disabled_by_jvm_flags(vmIntrinsics::ID id) { | |||
| case vmIntrinsics::_fmaF: | |||
| if (!InlineMathNatives || !UseFMA) return true; | |||
| break; | |||
| case vmIntrinsics::_dtanh: | |||
| case vmIntrinsics::_dcbrt: | |||
| if (!InlineMathNatives) return true; | |||
There was a problem hiding this comment.
Ah, looks like you missed !InlineIntrinsic check here:
if (!InlineMathNatives || !InlineIntrinsics) return true;
There was a problem hiding this comment.
Yes, I was thinking to fall through to the cases below, but I missed the break statement. It's clearer with the extra check anyway, so I added it in now.
Yes it does - thanks. Though I'm still unclear why only a couple of these math intrinsics are affected by |
The unaffected math intrinsics (dsin, dcos, dexp, ...) can fall back to alternate C++ implementations in the shared runtime when the stubs aren't generated by |
| @@ -316,6 +314,10 @@ bool vmIntrinsics::disabled_by_jvm_flags(vmIntrinsics::ID id) { | |||
| case vmIntrinsics::_fmaF: | |||
| if (!InlineMathNatives || !UseFMA) return true; | |||
| break; | |||
| case vmIntrinsics::_dtanh: | |||
| case vmIntrinsics::_dcbrt: | |||
| if (!InlineMathNatives || !InlineIntrinsics) return true; | |||
There was a problem hiding this comment.
You also need to add check for UseLibmIntrinsic here:
#ifdef AMD64
if (!UseLibmIntrinsic) return true;
#endif
@sviswa7 I had to make some more changes to cover the linux-x64-hs-zero build. |
This change fixes unwanted calls to the cbrt and tanh intrinsics on x86 when they are explicitly disabled by flags such as -XX:-UseLibmIntrinsic or -XX:-InlineIntrinsics.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25680/head:pull/25680$ git checkout pull/25680Update a local copy of the PR:
$ git checkout pull/25680$ git pull https://git.openjdk.org/jdk.git pull/25680/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25680View PR using the GUI difftool:
$ git pr show -t 25680Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25680.diff
Using Webrev
Link to Webrev Comment