8382713: [VectorAPI] Perform late inlining of failed vector intrinsics - #30876
8382713: [VectorAPI] Perform late inlining of failed vector intrinsics#30876jatin-bhateja wants to merge 20 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. |
…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?
There was a problem hiding this comment.
Please refer to root cause and suggestions from Vladimir.
#30876 (comment)
#30876 (comment)
| .addFlags("--add-modules=jdk.incubator.vector", | ||
| "-XX:-IncrementalInlineVector") |
There was a problem hiding this comment.
Same question about flag here.
There was a problem hiding this comment.
Please refer to
#30876 (comment)
#30876 (comment)
There is also a follow up issue JDK-8385134 filed for it
| .addFlags("--add-modules=jdk.incubator.vector", | ||
| "-XX:-IncrementalInlineVector") |
There was a problem hiding this comment.
Same question about flag here.
There was a problem hiding this comment.
AFAIR I looked at this particular one (not sure about other ones) and the culprit was the code inlined as part of fallback implementation interfered with the IR test assumptions (unexpected nodes were present). So, disabling inlining is a valid fix for the immediate problem.
There was a problem hiding this comment.
Hmm. But is that really good design @iwanowww ? Should we not have some comments that explain this?
These tests are here to ensure we have the expected shapes under regular production - what's the benefit of having IR rules that only apply to non-standard execution?
There was a problem hiding this comment.
Comments would help, for sure.
I see it as a test issue. IR tests are whitebox in nature and carefully control the mode they execute in to preserve the invariants they test. Ideally, in this particular case, the tests should detect failed intrinsification attempts and treat them as test failures.
what's the benefit of having IR rules that only apply to non-standard execution?
We either have to explicitly enumerate all supported configurations (supporting reliable intrinsification) or disable execution modes which break test assumptions (the approach taken here). I suggest to file a test bug to improve affected tests. For now, preserving original behavior (no late inlining of fallback implementation) looks fine.
There was a problem hiding this comment.
I at least need comments. I don't want to have to reverse engineer why we need that flag there.
And I would like to see IR rules for the new default situation now. That would really help me understand the impact on those tests - otherwise I don't fully know what I'm approving here ;)
Enumerating all cases is sometimes helpful.
There was a problem hiding this comment.
Your suggestion works cleanly for TestVectorTest — done. That failure is deterministic and bound to a static condition: under UseAVX=0 the VectorMask.fromLong/fromBitsCoerced intrinsic doesn't apply, the fallback is inlined, and it materializes the mask query with the scalar CmpI/CMoveI the rules forbid. Under UseAVX>0 it intrinsifies cleanly. So I dropped -XX:-IncrementalInlineVector there and guarded the rules per-platform exactly as you proposed
Great to hear that worked!
For VectorMaskCompareNotTest / VectorCompareWithZeroTest [...]
Empirically, running feature-on (no -XX:-IncrementalInlineVector):
- Default config: 0 failures.
- Stress config (-XX:CompileThreshold=100 -XX:-TieredCompilation), four byte-for-byte identical runs: 1 → 84 → 0 → 0 failing methods, with the failing method changing run-to-run.
Thanks for the explanation @jatin-bhateja !
Can you have an applyIf that checks for "normal" CompileThreshold/TieredCompilation, so we get the profiling we get under default conditions? Have you tried that yet?
On how to actually handle this test bug
I still don't understand why we classify this as some kind of "special test bug", that is somehow not related to the changes here. If affects inlining, and we see the consequences in the IR. Looking at the IR effects of this patch here seems relevant, isn't it? What we see here, for example, is that the compilation output is very profiling sensitive now, maybe more than before? But maybe that only happens under the lowered CompileThreshold, which makes profiling less stable, so I suppose that's ok? What do you think @iwanowww ?
There was a problem hiding this comment.
It could also be good to add comments to the methods that get flaky compilation, especially if the compilation should show to stay flaky under default compilation. Because that would be more than a test-bug.
There was a problem hiding this comment.
Hi @eme64 , I have removed -XX:-IncrementalInlineVector from both VectorMaskCompareNotTest and VectorCompareWithZeroTest tests and modified the IR rules as suggested.
I hope that addresses your concern since tests are now enabled under default settings of IncrementalInlineVector.
There was a problem hiding this comment.
@jatin-bhateja Excellent. I'm glad this was possible!
Do we now still need your test-bug follow-up?
https://bugs.openjdk.org/browse/JDK-8388060
There was a problem hiding this comment.
@jatin-bhateja Excellent. I'm glad this was possible!
Do we now still need your test-bug follow-up? https://bugs.openjdk.org/browse/JDK-8388060
Current test modification pretty much cover what was planned for follow up test bug.
@eme64 what kind of regressions do you have in mind? |
@iwanowww I'd like to have tests that prevent a later change from accidentally regressing our improvements from this PR here. |
Got it. Sounds reasonable. |
Hi @eme64 , I have added an IR verification test which exercises both the scenarios with and without IncrementalInlineVector. |
| if (IncrementalInlineVector && allow_inline) { | ||
| // Try to late inline fallback implementation if intrinsification attempt fails. | ||
| CallGenerator* fallback_cg; | ||
| { | ||
| InlinePrinterSuspendScope guard(C->inline_printer()); | ||
| fallback_cg = call_generator(callee, vtable_index, call_does_dispatch, jvms, | ||
| true /*allow_inline*/, prof_factor, | ||
| speculative_receiver_type, false /*allow_intrinsics*/); | ||
| } | ||
| if (fallback_cg != nullptr && fallback_cg->is_parse()) { | ||
| return CallGenerator::for_vector_late_inline(callee, cg, fallback_cg); |
There was a problem hiding this comment.
Question on naming: is it "late" or "incremental" inlining? Does it make sense to have both names?
There was a problem hiding this comment.
You now just updated one comment, right?
I am still wondering about IncrementalInlineVector vs for_vector_late_inline.
Is there a reason for this?
There was a problem hiding this comment.
Hi @eme64 ,
This is as per existing naming in Hotspot, an existing flag IncrementalInline has its corresponding call generator named LateInlineCallGenerator which is instantiated by CallGenerator::for_late_inline.
| * A 512-bit FloatVector operation cannot be intrinsified when AVX-512 is not used | ||
| * (here forced with -XX:UseAVX=2), so the vector intrinsic fails to expand. With | ||
| * IncrementalInlineVector enabled (the default) the fallback implementation | ||
| * (VectorSupport::binaryOp) is inlined into the caller, absorbing the call | ||
| * overhead; with the flag disabled, a static call to the fallback remains. |
There was a problem hiding this comment.
If we ever do vector-splitting, we could actuall inline 512-bit vectors if we know that we can split it to 256-bit or 128-bit vectors later. That's actually something we would like to do in the future. If we get to that, then this test will need to be rewritten. Is that something you are aware of?
There was a problem hiding this comment.
Modified the test.
| @Test | ||
| @IR(failOn = { IRNode.VMASK_CMP_ZERO_L_NEON }) | ||
| @IR(failOn = { IRNode.VMASK_CMP_ZERO_L_NEON }, | ||
| applyIfAnd = { "TieredCompilation", "true", "CompileThreshold", "10000" }) |
There was a problem hiding this comment.
There is a risk that we will at some point tune CompileThreshold, to let's say 11000 or 9000. And then all of these IR rules will never be executed ever again. How high does the CompileThreshold really need to be to get stable compilation?
There was a problem hiding this comment.
It seems that CompileThreshold will not impact the test runs, since framework strips it off.
https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/driver/TestVMProcess.java#L109
I have removed it from IR rules.
|
Hi @eme64 , your comments have been addressed. |
|
@jatin-bhateja this one is still pending for me: #30876 (comment) I'm also wondering: do we have some benchmarks that benefit from this change, and would justify this extra complexity? There must have been some benchmark that motivated this work, right? It would be good to see something in the PR description. |
Hi @eme64, I've included the results of the BlackScholes benchmark runs in the PR description. As noted there, we also have a dependent pull request #24104 whose fallback implementation is itself built on the Vector API — inlining that fallback is what lets us avoid the boxing penalties, which is a key performance motivation for this change. |
|
Hi @eme64 , your comments have been addressed/answered. Looking forward to your approval |
|
Hi @XiaohongGong, can you kindly review this pull request. |
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.
I modified BackSholes benchmark to use FloatVector.SPECIES_512, and then explicitly passed -XX:UseAVX=2 to force intrinsic failure. Following are the performance numbers with and without InlineVectorFallback
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
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