jdk
jdk copied to clipboard
openjdk
8289422: Fix and re-enable vector conditional move
// float[] a, float[] b, float[] c;
for (int i = 0; i < a.length; i++) {
c[i] = (a[i] > b[i]) ? a[i] : b[i];
}
After JDK-8139340 and JDK-8192846, we hope to vectorize the case above by enabling -XX:+UseCMoveUnconditionally and -XX:+UseVectorCmov. But the transformation here[1] is going to optimize the BoolNode with constant input to a constant and break the design logic of cmove vector node[2]. We can't prevent all GVN transformation to the BoolNode before matcher, so the patch keeps the condition input as a constant while creating a cmove vector node, and then restructures it into a binary tree before matching.
When the input order of original cmp node is different from the input order of original cmove node, like:
// float[] a, float[] b, float[] c;
for (int i = 0; i < a.length; i++) {
c[i] = (a[i] < b[i]) ? a[i] : b[i];
}
the patch negates the mask of the BoolNode before creating the cmove vector node in SuperWord::output().
We can also use VectorNode::implemented() to consult if vector conditional move is supported in the backend. So, the patch cleans the related code in SuperWord::implemented().
With the patch, the performance uplift is: (The micro-benchmark functions are included in the file test/micro/org/openjdk/bench/vm/compiler/TypeVectorOperations.java)
AArch64: Benchmark (length) Mode Cnt uplift(ns/op) cmoveD 523 avgt 15 68.89% cmoveF 523 avgt 15 72.40%
X86: Benchmark (length) Mode Cnt uplift(ns/op) cmoveD 523 avgt 15 73.12% cmoveF 523 avgt 15 85.45%
[1]https://github.com/openjdk/jdk/blob/779b4e1d1959bc15a27492b7e2b951678e39cca8/src/hotspot/share/opto/subnode.cpp#L1310 [2]https://github.com/openjdk/jdk/blob/779b4e1d1959bc15a27492b7e2b951678e39cca8/src/hotspot/share/opto/matcher.cpp#L2365
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8289422: Fix and re-enable vector conditional move
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9652/head:pull/9652
$ git checkout pull/9652
Update a local copy of the PR:
$ git checkout pull/9652
$ git pull https://git.openjdk.org/jdk pull/9652/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9652
View PR using the GUI difftool:
$ git pr show -t 9652
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9652.diff
:wave: Welcome back fgao! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@fg1417 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.
Webrevs
- 04: Full (678314e1)
- 03: Full (8bb57783)
- 02: Full (7aa3adf1)
- 01: Full - Incremental (3c839000)
- 00: Full (b2daf42b)
@fg1417
The label rfr is not a valid label.
These labels are valid:
-
serviceability -
hotspot -
hotspot-compiler -
ide-support -
kulla -
i18n -
shenandoah -
jdk -
javadoc -
security -
hotspot-runtime -
jmx -
build -
nio -
client -
core-libs -
compiler -
net -
hotspot-gc -
hotspot-jfr
I can run this through our testing but please resolve the merge conflict first.
I can run this through our testing but please resolve the merge conflict first.
Thanks @TobiHartmann. I'll fix the conflict ASAP.
Thanks, I can see failures with the following tests when running with -XX:+UseCMoveUnconditionally -XX:+UseVectorCmov:
-
compiler/c2/TestCondAddDeadBranch.java -
compiler/loopopts/TestCastFFAtPhi.java
Error mixing types: vectory[4]:{double_top} and double_top
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (workspace/open/src/hotspot/share/opto/type.cpp:1179), pid=3589333, tid=3589359
# Error: ShouldNotReachHere()
#
# JRE version: Java(TM) SE Runtime Environment (20.0) (fastdebug build 20-internal-2022-09-09-0957028.tobias.hartmann.jdk2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 20-internal-2022-09-09-0957028.tobias.hartmann.jdk2, compiled mode, sharing, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V [libjvm.so+0x1a95869] Type::typerr(Type const*) const+0x79
Current CompileTask:
C2: 130 10 b TestCastFFAtPhi::init (35 bytes)
Stack: [0x00007ff917726000,0x00007ff917827000], sp=0x00007ff917821540, free space=1005k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x1a95869] Type::typerr(Type const*) const+0x79 (type.cpp:1179)
V [libjvm.so+0x1a97f2b] TypeVect::xmeet(Type const*) const+0x1eb (type.cpp:2451)
V [libjvm.so+0x1a9d203] Type::meet_helper(Type const*, bool) const+0x73 (type.cpp:879)
V [libjvm.so+0x1a9d41a] Type::filter_helper(Type const*, bool) const+0x1a (type.hpp:188)
V [libjvm.so+0x1793690] PhaseIterGVN::transform_old(Node*)+0x230 (phaseX.cpp:1294)
V [libjvm.so+0x178b30e] PhaseIterGVN::optimize()+0x6e (phaseX.cpp:1203)
V [libjvm.so+0xafeefa] PhaseIdealLoop::optimize(PhaseIterGVN&, LoopOptsMode)+0x6da (loopnode.hpp:1169)
V [libjvm.so+0xafb253] Compile::Optimize()+0xe53 (compile.cpp:2171)
V [libjvm.so+0xafd50d] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x15ad (compile.cpp:823)
V [libjvm.so+0x90e2e5] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x675 (c2compiler.cpp:113)
V [libjvm.so+0xb0ba5c] CompileBroker::invoke_compiler_on_method(CompileTask*)+0xb1c (compileBroker.cpp:2243)
V [libjvm.so+0xb0c828] CompileBroker::compiler_thread_loop()+0x5a8 (compileBroker.cpp:1917)
V [libjvm.so+0x106c1dc] JavaThread::thread_main_inner()+0x22c (javaThread.cpp:700)
V [libjvm.so+0x1a6dd10] Thread::call_run()+0x100 (thread.cpp:224)
V [libjvm.so+0x1708f13] thread_native_entry(Thread*)+0x103 (os_linux.cpp:710)
They also happen without this patch. Should we file a separate bug or are these supposed to be fixed by this change?
Thanks, I can see failures with the following tests when running with
-XX:+UseCMoveUnconditionally -XX:+UseVectorCmov:
compiler/c2/TestCondAddDeadBranch.javacompiler/loopopts/TestCastFFAtPhi.javaThey also happen without this patch. Should we file a separate bug or are these supposed to be fixed by this change?
Thanks for your effort, @TobiHartmann . The backtrace of the failure is different from the problem that the patch tries to fix. It may be caused by another problem in mid-end. So I prefer to fix it in a separate patch and try to make each patch much easier. WDYT?
Okay, please go ahead and file a follow-up bug then.
Sure. I filed a new JBS issue in https://bugs.openjdk.org/browse/JDK-8293833.
@fg1417 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:
8289422: Fix and re-enable vector conditional move
Reviewed-by: thartmann, kvn
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 27 new commits pushed to the master branch:
- 1ddc92fef518cbbb06945f7b5a1e285f740682cb: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
- 1e222bccd3807c1be0d1d824e0ff9745751d8375: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
- 43eff2b309e2ef275bdd5adf196da81d4e23f535: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
- b88ee1ee22a4ea859f2a7bdf80a12c1d56fe6fd2: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
- aca4276e8938127e7e6a416cfbe325764b2c2e3f: 8294379: Missing comma after copyright year
- 1f521a12041b33b3458f952627d535fad6e928c7: 8225012: sanity/client/SwingSet/src/ToolTipDemoTest.java fails on Windows
- 5ae6bc23e857535532b59aae674e2b917bbf7284: 8234262: Unmask SIGQUIT in a child process
- 968af74de4307a05e45f0bee32fa9120e39faf09: 8293567: AbstractSplittableWithBrineGenerator: salt has digits that duplicate the marker
- 36b61c5d7e7732924f494fa24c0e286e41279fc3: 8293872: Make runtime/Thread/ThreadCountLimit.java more robust
- 2be315877b734b70170ef6375712188d7cd64268: 4797982: Setting negative size of JSplitPane divider leads to unexpected results.
- ... and 17 more: https://git.openjdk.org/jdk/compare/a4dc035a9731a32083bbd3fa28408bfaa3474b54...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 (@TobiHartmann, @vnkozlov) 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).
@TobiHartmann The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
Changes looks good. Please, add testing for swapped inputs (when you negate condition).
Thanks for your comment @vnkozlov . Updated it with some new tests.
Thanks for your test work @vnkozlov .
Also thanks all for your review and comments. I'll integrate it.
/integrate
@fg1417 Your change (at version 678314e1ef6000da3cd5ce117b5b051410231546) is now ready to be sponsored by a Committer.
Going to push as commit aa48705dddee674baa479f5128cfc3b426d87d2d.
Since your change was applied there have been 27 commits pushed to the master branch:
- 1ddc92fef518cbbb06945f7b5a1e285f740682cb: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
- 1e222bccd3807c1be0d1d824e0ff9745751d8375: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
- 43eff2b309e2ef275bdd5adf196da81d4e23f535: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
- b88ee1ee22a4ea859f2a7bdf80a12c1d56fe6fd2: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
- aca4276e8938127e7e6a416cfbe325764b2c2e3f: 8294379: Missing comma after copyright year
- 1f521a12041b33b3458f952627d535fad6e928c7: 8225012: sanity/client/SwingSet/src/ToolTipDemoTest.java fails on Windows
- 5ae6bc23e857535532b59aae674e2b917bbf7284: 8234262: Unmask SIGQUIT in a child process
- 968af74de4307a05e45f0bee32fa9120e39faf09: 8293567: AbstractSplittableWithBrineGenerator: salt has digits that duplicate the marker
- 36b61c5d7e7732924f494fa24c0e286e41279fc3: 8293872: Make runtime/Thread/ThreadCountLimit.java more robust
- 2be315877b734b70170ef6375712188d7cd64268: 4797982: Setting negative size of JSplitPane divider leads to unexpected results.
- ... and 17 more: https://git.openjdk.org/jdk/compare/a4dc035a9731a32083bbd3fa28408bfaa3474b54...master
Your commit was automatically rebased without conflicts.
@pfustc @fg1417 Pushed as commit aa48705dddee674baa479f5128cfc3b426d87d2d.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.