8382713: [VectorAPI] Perform late inlining of failed vector intrinsics#30876
8382713: [VectorAPI] Perform late inlining of failed vector intrinsics#30876jatin-bhateja wants to merge 15 commits into
Conversation
|
/label add hotspot-compiler-dev |
|
👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@jatin-bhateja |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
|
@jatin-bhateja To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command. Applicable Labels
|
Webrevs
|
| } | ||
| }; | ||
|
|
||
| bool LateInlineVectorCallGenerator::inline_fallback() const { |
There was a problem hiding this comment.
What's the purpose of this method? All vector intrinsics do have fallback implementation. If there are any cases added later, then they don't have to rely on LateInlineVectorCallGenerator.
|
Hi @iwanowww , your comments have been addressed. |
|
I modified BackSholes benchmark to use FloatVector.SPECIES_512, and then explicitly passed CommandLine: java -jar target/benchmarks.jar -f 1 -i 5 -wi 1 -w 30 -jvmArgs "-XX:UseAVX=2 --add-modules=jdk.incubator.vector -XX:+UnlockDiagnosticVMOptions -XX:+InlineVectorFallback" BlackScholes.vector_black_scholes |
|
Hi @iwanowww , your comments have been addressed. |
iwanowww
left a comment
iwanowww
left a comment
There was a problem hiding this comment.
Overall, looks good. Minor suggestions follow.
| product(bool, EnableVectorAggressiveReboxing, false, EXPERIMENTAL, \ | ||
| "Enables aggressive reboxing of vectors") \ | ||
| \ | ||
| product(bool, InlineVectorFallback, true, DIAGNOSTIC, \ |
There was a problem hiding this comment.
Let's call it IncrementalInlineVector and put it next to IncrementalInline et al.
|
Hi @iwanowww , your comments have been addressed, please share the results of your test run. |
|
Unfortunately, I see multiple failures in Vector API-related tests. They failed mostly on linux-aarch64, but there were few linux-x64 failures [1] as well. I'll take a closer look, but it seems the problem on linux-aarch64 is that fallback implementations are unconditionally inlined and it causes problems (multiple tests on 512-bit vectors fail due memory exhaustion [2]). [1] In particular:
[2] jdk/incubator/vector/ByteVector512LoadStoreTests.java |
Hi @iwanowww , this failure is related to use of UseAVX=0, here fromBitsCoerced is not intrinsified, earlier it remained as CallStaticJavaNode but now it gets inlined, new inlined context has graph shape which infers CMoveI and CmpI and test failed since IR rule don't expect these nodes, one target agnostic fix is to guard these IR rules with -XX:-IncrementalInlineVector flag, but it will defeat the purpose of this test since IncrementalInlineVector is default on. Since test runs on multiple targets guarding by UseAVX > 0 may not be desirable. Let me know what do you think ?
With the patch, the vector intrinsic fallback inlining generates more code in the compilation unit, this effects inlining of This increase the outcout of VectorMaskCmpNode and inhabits optimization which folds XorVMask (VectorMaskCmp, maskAll(true) Increasing InlineSmallCode to 10000 allows intoArray to be inlined, mask is not boxed, VectorMaskCmp has outcnt=1, XorVMask is folded and Test Passes
Over all there is a tradeoff of unconditionally inlining vector intrinsic since most of them a bulky and it may impact inlining decisions within their calling context. Do you think its beneficial to limit the scope of inlining to only few intrinsics initially e.g. Please let me know your views. |
|
@jatin-bhateja Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
I don't see why it defeats the purpose of the test. It's an IR test and limiting possible IR shapes is fine.
Do we miss
I think regular inlining heuristics should be applied to vector fallback implementations. |
| @@ -47,16 +47,16 @@ public static void main(String[] args) { | |||
| public int call() { return 1; } | |||
|
|
|||
| @Test | |||
| @IR(failOn = {IRNode.CMP_I, IRNode.CMOVE_I}) | |||
| @IR(failOn = {IRNode.CMP_I, IRNode.CMOVE_I}, applyIf = {"IncrementalInlineVector", "false"}) | |||
There was a problem hiding this comment.
Does it mean that the rule is disabled unless the test is explicitly run with -XX:-IncrementalInlineVector?
I doubt it will be regularly executed in such mode. So, it defeats the purpose of the test, doesn't it?
Instead, why don't you explicitly run the test with -XX:-IncrementalInlineVector flag?
There was a problem hiding this comment.
Done, I am now passing -XX:-IncrementalInlineVector to test invocation.
| @@ -1294,7 +1294,7 @@ public static void testCompareMaskNotDoubleNegative() { | |||
| public static void main(String[] args) { | |||
| TestFramework testFramework = new TestFramework(); | |||
| testFramework.setDefaultWarmup(5000) | |||
| .addFlags("--add-modules=jdk.incubator.vector") | |||
| .addFlags("--add-modules=jdk.incubator.vector", "-XX:InlineSmallCode=100000") | |||
There was a problem hiding this comment.
Should AbstractMask::intoArray() be marked w/ @ForceInline instead?
There was a problem hiding this comment.
With @ForceInline over AbstractMask::intoArray test passes with "-ea -esa -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation" but fails with default option due to difference in inlining
Failed IR Rules (1) of Methods (1)
----------------------------------
1) Method "compiler.vectorapi.VectorMaskCompareNotTest::testCompareNEMaskNotFloatNaN" - [Failed IR rules: 1]:
* @IR rule 1: "@compiler.lib.ir_framework.IR(phase={DEFAULT}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={"asimd", "true", "avx", "true", "rvv", "true"}, counts={"_#XOR_V_MASK#_", "= 0", "_#XOR_V#_", "= 0", "_#VECTOR_MASK_CMP#_", "= 2"}, failOn={}, applyIfPlatform={}, applyIfPlatformOr={}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
> Phase "PrintIdeal":
- counts: Graph contains wrong number of nodes:
* Constraint 3: "(\d+(\s){2}(VectorMaskCmp.*)+(\s){2}===.*)"
- Failed comparison: [found] 0 = 2 [given]
- No nodes matched!
With -XX:InlineSmallCode=1000000 it passes with all the configurations.
There was a problem hiding this comment.
Please, elaborate where it fails. Does func.apply(m).intoArray(mr, 0); in testCompareMaskNotFloat cause problems?
There was a problem hiding this comment.
I investigated if further here is my analysis
Adding @ForceInline to AbstractMask::intoArray is desirable for vector intrinsic inlining, but it exposes a pre-existing bug in C2's switch profiling.
The bug is in Parse::do_tableswitch() in parse2.cpp: when a mature MDO has all-zero MultiBranchData counts, merge_ranges() marks every arm as never_reached, and jump_switch_ranges() collapses the entire switch to a single unstable_if trap. The parser should treat this as "no useful profile" (fall back to cnt = 1.0F), not "every arm is cold." I confirmed this analysis by passing -XX:-TieredCompilation or -XX:-UseSwitchProfiling — the test passes with either flag.
This profiling issue is orthogonal to the vector intrinsic late inlining work and should be addressed in a separate PR. For now, @ForceInline on AbstractMask::intoArray is not added and -XX:InlineSmallCode=1000000 is added to the failing test as a workaround
There was a problem hiding this comment.
Thanks for the details. Hm, that doesn't sound right. There's no support for caller-sensitive profiling yet, so each method profile data is stored in a dedicated per-method MDO instance. (There are deoptimization counts which may depend on inlining, but regular branch counts should not be affected.) Anyway, let's continue investigating it separately.
Please, file a follow-up bug for it. Does -XX:-IncrementalInlineVector work as a workaround? I'm not fond of InlineSmallCode tweaks.
There was a problem hiding this comment.
I have filed a follow up JBS for this https://bugs.openjdk.org/browse/JDK-8385134
| @@ -1294,7 +1294,7 @@ public static void testCompareMaskNotDoubleNegative() { | |||
| public static void main(String[] args) { | |||
| TestFramework testFramework = new TestFramework(); | |||
| testFramework.setDefaultWarmup(5000) | |||
| .addFlags("--add-modules=jdk.incubator.vector") | |||
| .addFlags("--add-modules=jdk.incubator.vector", "-XX:InlineSmallCode=100000") | |||
There was a problem hiding this comment.
Please, elaborate where it fails. Does func.apply(m).intoArray(mr, 0); in testCompareMaskNotFloat cause problems?
|
Overall, looks good. I tweaked [1] |
|
One more IR test failure: |
|
@jatin-bhateja this pull request can not be integrated into git checkout JDK-8382713
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
Bot
added
the
merge-conflict
…ing fallback call generator selection
openjdk
Bot
removed
the
merge-conflict
|
Hi @iwanowww , added the handling to prevent accumulation of spurious messages during fallback call generator selection using RAII based mechanism. Also explicitly printing message Please let me if the patch looks good land now, your earlier comments have been addressed. |
| return &_nullStream; | ||
| } | ||
| if (is_suspended()) { | ||
| locate(state, callee); |
There was a problem hiding this comment.
Why do you perform locate call?
There was a problem hiding this comment.
locate() was retained to keep the IPInlineSite topology intact (one node per JVMState frame) while only the message append is suppressed. But that topology is already guaranteed by record ordering — the order in which record() calls happen during compilation ensures every node's parent exists before the node itself is needed — so locate() rebuilding the path during a suspended probe is unnecessary, and dropping it is safe.
Hi @iwanowww , Your comments have been addressed. |
iwanowww
left a comment
iwanowww
left a comment
There was a problem hiding this comment.
Looks good. Submitted for testing.
|
Strangely, |
Rebased with latest mainline, looks like it mistakenly got introduced with previous merge. Kindly re-verify. |
|
Hi @mhaessig , we need one more approval here to transition this to ready state, can you do the needful. |
eme64
left a comment
eme64
left a comment
There was a problem hiding this comment.
@jatin-bhateja This looks like important work, so thanks for working on it!
I have some questions about the tests below, I'm especially wondering why you had to set -XX:-IncrementalInlineVector in some of the IR tests? Because if the flag is now on by default, would it not be more important to have IR rules with the flag enabled? What are the affected IR rules?
Also: Could we have some new IR tests that demonstrate the benefit of late vector inlining, and make sure there won't be regressions on it?
| public static void main(String[] args) { | ||
| TestFramework.runWithFlags("--add-modules=jdk.incubator.vector"); | ||
| TestFramework.runWithFlags("--add-modules=jdk.incubator.vector", | ||
| "-XX:-IncrementalInlineVector"); |
There was a problem hiding this comment.
Why did you add these flags here? Would the IR rules fail without?
Suggestion: can you have a run with and a run without the flag, and then show which IR rules are affected, guarding them with the flag?
| .addFlags("--add-modules=jdk.incubator.vector", | ||
| "-XX:-IncrementalInlineVector") |
There was a problem hiding this comment.
Same question about flag here.
| .addFlags("--add-modules=jdk.incubator.vector", | ||
| "-XX:-IncrementalInlineVector") |
There was a problem hiding this comment.
Same question about flag here.
3 participants
Currently, we attempt lazy intrinsification of vector intrinsics during incremental inlining stage, in case intrinsification fail due to non-constant context expected by the inline expander, a static call is generated, this incurs a call overhead penalty.
As per following comments from @iwanowww on JDK-8303762 pull request
#24104 (comment)
We should attempt procedure inlining of failed vector intrinsics to avoid penalties associated with call overhead, for vector operations whose fall back implementation uses other vector APIs it will also save boxing penalty.
Patch address this concern by adding a new hybrid call generator (LateInlineVectorCallGenerator ) which encapsulates both intrinsic and parser call generator. During incremental inlining, the intrinsic gets multiple chances to succeed. If all attempts fail, the fallback implementation is inlined instead, absorbing call over head penalties.
Please review and share your feedback.
Best Regards,
Jatin
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30876/head:pull/30876$ git checkout pull/30876Update a local copy of the PR:
$ git checkout pull/30876$ git pull https://git.openjdk.org/jdk.git pull/30876/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30876View PR using the GUI difftool:
$ git pr show -t 30876Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30876.diff
Using Webrev
Link to Webrev Comment