jdk
jdk copied to clipboard
openjdk
8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value
This PR resolves JDK-8327381
Currently the transformations for expressions with patterns ((x & m) u<= m) or ((m & x) u<= m) to true is in BoolNode::Ideal function with a new constant node of value 1 created. However, this is technically a type-improving (reduction in range) transformation that's better suited in BoolNode::Value function.
New unit test test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java asserting on IR nodes and correctness of this transformation is added and passing.
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Issue
- JDK-8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value (Enhancement - P5)
Reviewers
- Jasmine Karthikeyan (@jaskarth - Committer) 🔄 Re-review required (review applies to e2eb8bf9)
- Emanuel Peter (@eme64 - Reviewer) 🔄 Re-review required (review applies to 84784c74)
- Christian Hagedorn (@chhagedorn - Reviewer) 🔄 Re-review required (review applies to 84784c74)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18198/head:pull/18198
$ git checkout pull/18198
Update a local copy of the PR:
$ git checkout pull/18198
$ git pull https://git.openjdk.org/jdk.git pull/18198/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18198
View PR using the GUI difftool:
$ git pr show -t 18198
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18198.diff
Webrev
:wave: Welcome back kxu! 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}
@tabjy 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
- 13: Full - Incremental (719199c2)
- 12: Full - Incremental (29655d35)
- 11: Full - Incremental (b731f6d7)
- 10: Full - Incremental (715a6304)
- 09: Full - Incremental (84784c74)
- 08: Full - Incremental (278c436a)
- 07: Full - Incremental (53cf5b3b)
- 06: Full - Incremental (ae5bed23)
- 05: Full - Incremental (02c01edf)
- 04: Full - Incremental (47d0172b)
- 03: Full - Incremental (e2eb8bf9)
- 02: Full - Incremental (06b7da36)
- 01: Full - Incremental (17a9dc37)
- 00: Full (aa7fafb8)
Thanks @jaskarth and @eme64 for the review. I've pushed a new commit to address the following:
- Updated license header year to 2024
- Explicit
nullptrcomparison -
Node* varfor pointer types - Test moved to
c2.irTests, added@bugand@summarytags
@tabjy 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:
8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value
Reviewed-by: chagedorn, thartmann, jkarthikeyan, epeter
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 28 new commits pushed to the master branch:
- 32c975098521e830ce706b67e7232a007c0846c7: 8339160: [BACKOUT] JDK-8338440 Parallel: Improve fragmentation mitigation in Full GC
- 9d183bd02763ee4ff5aa8388e039d8b5a6964328: 8339149: jfr_flush_event_writer - return value type mismatch
- 2150521650d6b730cfe9d3ecb91d589c96862475: 8322036: Improve help output from the javadoc tool
- 1ff9ac7233d51a58fd54a92d2c45761478574cc7: 8338731: MemoryLayout::offsetHandle can return a negative offset
- 2e174c6367c7755d8541f9669f7f10ed89878f58: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest
- 8e88da05b9966892e117b779d59a2e311a557a8d: 8338041: Keyboard Navigation of JTable, Ctrl Shift RIGHT/LEFT doesn't follow native action in GTK L&F
- 449ca2c3c1cb5d056a2d259be2ff069ba2a36b80: 8337832: Optimize datetime toString
- b1b4cd429a4135840966975dd0c068fe428e2ee6: 8332158: [XWayland] test/jdk/java/awt/Mouse/EnterExitEvents/ResizingFrameTest.java
- 284c3cde5e1b7115fb17c51f3ed17c1be95845bc: 8336299: Improve GCLocker stall diagnostics
- 2e96f159aaee782a627902c04dbd51daa3406ab5: 8338489: Typo in MemorySegment doc
- ... and 18 more: https://git.openjdk.org/jdk/compare/a15af6998e8f7adac2ded94ef5a47e22ddb53452...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 (@jaskarth, @eme64, @chhagedorn, @TobiHartmann) 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).
Ah, I had assumed the transformation was valid beforehand, since it had existed for a while :sweat_smile: The issue only impacts -1, right? Since the comparison should succeed for both m >=0 and m < -1. I think it would be good to address it in this patch, as it's refactoring the existing code.
The original patch seems to primarily test with array.length as the m value, so the value set was nonnegative. I think we can limit the ((x & m) u< m + 1) transform to cases where m is known to be nonnegative and maintain the intent behind the transform. Something like:
-} else if (_test._test == BoolTest::lt && cmp2->Opcode() == Op_AddI && cmp2->in(2)->find_int_con(0) == 1) {
+} else if (_test._test == BoolTest::lt && cmp2->Opcode() == Op_AddI && cmp2->in(2)->find_int_con(0) == 1 && phase->type(cmp2->in(1))->is_int()->_lo >= 0) {
With the IR test being modified accordingly. It'd also be good to write an IR test method that verifies that the transform doesn't take place if m doesn't succeed the _lo >= 0 test.
@jaskarth Optimally, (x & m) u< m + 1 can be transformed into m != -1 but I think limiting it to non-negative m seems to be a reasonable approach.
@eme64 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 2 Reviewers).
Sorry for not following up closely. The PR has been updated per review suggested.
Hi @eme64, I'm wondering if you could kindly take a look at the updated commits. Thanks a lot!
(x & m) u< m + 1is false form = -1, right?
This bug should be handled separately. I'll do that.
/reviewers 2 reviewer
@chhagedorn told me he would also like to review this. I will also run testing again.
@eme64 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 2 Reviewers).
PR updated according to requests. It looked like GHA previously had a problem with serviceability tests across the repo. Let's wait until the new round of testing is complete.
I've run some testing again and the newly added test failed on all platforms in tier2:
compiler/c2/gvn/TestBoolNodeGVN.java
Additionally required flags:
-XX:-TieredCompilation
Maybe you need to increase the warm-up.
Output:
One or more @IR rules failed:
Failed IR Rules (1) of Methods (1)
----------------------------------
1) Method "public static boolean compiler.c2.gvn.TestBoolNodeGVN.testShouldHaveCpmU(int,int)" - [Failed IR rules: 1]:
* @IR rule 1: "@compiler.lib.ir_framework.IR(phase={AFTER_PARSING}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#CMP_U#_", "4"}, applyIfPlatform={}, failOn={}, applyIfPlatformOr={"x64", "true", "aarch64", "true", "riscv64", "true"}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
> Phase "After Parsing":
- counts: Graph contains wrong number of nodes:
* Constraint 1: "(\\d+(\\s){2}(CmpU.*)+(\\s){2}===.*)"
- Failed comparison: [found] 2 = 4 [given]
- Matched nodes (2):
* 31 CmpU === _ 23 27 [[ 32 ]] !jvms: TestBoolNodeGVN::testShouldHaveCpmU @ bci:9 (line 66)
* 60 CmpU === _ 23 57 [[ 61 ]] !jvms: TestBoolNodeGVN::testShouldHaveCpmU @ bci:44 (line 68)
@tabjy any update on this? Let us know if you need help with reproducing the issue that Christian reported. Thanks!
@TobiHartmann Thanks for the reminder.
I had no problem reproducing this issue, but what ultimately caused it is more involved and has something to do with a possible bug in the IR test framework itself. Let me explain:
First, enabling/disabling TieredCompilation does make a difference in produced IR. This has nothing to do # of executions during warm-up but rather the discrepancy in profiling: with TieredCompilation enlabled, profiling is done at C1 level, and there are 2 additional Cmp_U3 nodes and uncommon traps.
Second, the IR framework seems to be incorrectly matching Cmp_U3 as Cmp_U nodes. Afaik, the framework uses regex to match strings and determine the type of a node [0]. Because CmpU is also a prefix of CmpU3, there could be a problem with pattern matching. However, I've not confirmed this. If this is the case, it might deserve a separate issue.
Third, I made incorrect assumption on the number of Cmp_U nodes to begin with:
return !(Integer.compareUnsigned((x & m), m - 1) > 0) |
!(Integer.compareUnsigned((m & x), m - 1) > 0) |
Integer.compareUnsigned((x & m), m + 2) < 0 |
Integer.compareUnsigned((m & x), m + 2) < 0;
I was under the assumption that the 4 cases would produce 4 Cmp_U nodes; however, the first and the second are semantically equivelent and optmized into 1. The same goes for the latter two. With the additional 2 miscounted Cmp_U3, they coincidently add to 4 and passed test with TieredCompilation enabled. (But not with -XX:-TieredCompilation).
I pushed a commit to spread test cases compounded with & and | into subcases to avoid optimizing out semantically equivalent ones and to make test clearer. To make test passing even with CmpU3 miscounted as CmpU, I specified counts = {IRNode.CMP_U, ">=1"} instead of strictly 1. I hope this is acceptable.
A case illustrating CmpU3 matched as CmpU:
@Test
@Arguments(values = {Argument.DEFAULT, Argument.DEFAULT})
@IR(counts = {IRNode.CMP_U, "1"}, // <-- expecting strictly 1
phase = CompilePhase.AFTER_PARSING,
applyIfPlatformOr = {"x64", "true", "aarch64", "true", "riscv64", "true"})
public static boolean testShouldHaveCpmUCase1(int x, int m) {
return !(Integer.compareUnsigned((x & m), m - 1) > 0);
}
1) Method "public static boolean compiler.c2.gvn.TestBoolNodeGVN.testShouldHaveCpmUCase1(int,int)" - [Failed IR rules: 1]:
* @IR rule 1: "@compiler.lib.ir_framework.IR(phase={AFTER_PARSING}, applyIfPlatformAnd={}, applyIfCPUFeatureOr={}, counts={"_#CMP_U#_", "1"}, applyIfPlatform={}, failOn={}, applyIfPlatformOr={"x64", "true", "aarch64", "true", "riscv64", "true"}, applyIfOr={}, applyIfCPUFeatureAnd={}, applyIf={}, applyIfCPUFeature={}, applyIfAnd={}, applyIfNot={})"
> Phase "After Parsing":
- counts: Graph contains wrong number of nodes:
* Constraint 1: "(\d+(\s){2}(CmpU.*)+(\s){2}===.*)"
- Failed comparison: [found] 2 = 1 [given]
- Matched nodes (2):
mismatched--> * 28 CmpU3 === _ 23 27 [[ 39 ]] !jvms: TestBoolNodeGVN::testShouldHaveCpmUCase1 @ bci:6 (line 93)
* 31 CmpU === _ 23 27 [[ 32 ]] !jvms: TestBoolNodeGVN::testShouldHaveCpmUCase1 @ bci:9 (line 93)
Thanks @tabjy! @chhagedorn, our IR Framework expert, is currently on vacation but will be back later this week.
@chhagedorn should we change the regex to have a space at the end, so that we do not do this kind of prefix-matching?
@chhagedorn should we change the regex to have a space at the end, so that we do not do this kind of prefix-matching?
I think that would be a better solution to change the regex. Then we can change the test in such a way that it does an exact counting. Maybe we should move through all the IRNode regexes at some point and make sure that all placeholder strings match a unique node.
We already do that, for example, to match CountedLoop and not CountedLoopEnd by additionally matching the word boundary of the node name with \b:
https://github.com/openjdk/jdk/blob/05d88de05e9b7814ecd5517aacd17f0feafdff3c/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java#L495-L499
We could do something similar for CmpU (and we should then probably also apply it for CmpUL):
public static final String CMP_U = PREFIX + "CMP_U" + POSTFIX;
static {
String regex = START + "CmpU\\b" + MID + END;
beforeMatching(CMP_U, regex);
}
@tabjy Are you planning to keep working on this?
I talked with @chhagedorn and we would like you to change the IR rule to something like
@IR(counts = {IRNode.CMP_U + "\b", "1"}
In a later and separate RFE, we can then adjust the regex for all nodes, in a bulk update.
In a later and separate RFE, we can then adjust the regex for all nodes, in a bulk update.
Good idea, I filed JDK-8338809 to follow up on this after this PR gets integrated. But for now, I suggest to use an explicit regex as suggested above with @IR(counts = {IRNode.CMP_U + "\b", "1"}. We can then follow up and change this IR test once JDK-8338809 is tackled.
I sincerely apologize for not following up on this timely. This won't happen again.
[...] change the IR rule to something like
@IR(counts = {IRNode.CMP_U + "\b", "1"}
First, I assume you mean ... + "\\b" (double escaped for regex).
Unfortunately this does not work. IRNode.CMP_U has a postfix #_, making the expression _#CMP_U#_\\b.
https://github.com/openjdk/jdk/blob/a15af6998e8f7adac2ded94ef5a47e22ddb53452/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java#L440-L443
Even if we explicitly make it @IR(counts = {"_#CMP_U\\b#_", "1"}, it is still the one without the \b registered by beforeMatchingNameRegex(irNodePlaceholder, irNodeRegex), leading to unexpected node type during assertion:
Violations (4)
--------------
- IR Node "_#CMP_U\b#_" defined in class IRNode has no regex/compiler phase mapping (i.e. no static initializer block that adds a mapping entry to IRNode.IR_NODE_MAPPINGS).
Have you just created the entry "_#CMP_U\b#_" in class IRNode and forgot to add a mapping?
Violation for IR rule 1 at public static boolean compiler.c2.gvn.TestBoolNodeGVN.testShouldHaveCpmUCase1(int,int).
- [repeated violations omitted]
I think it's the second argument to beforeMatchingNameRegex(irNodePlaceholder, irNodeRegex) you want to add the word break to. Not the placeholder. This can only be done in IRNode.java. I propose the following change:
public static final String CMP_U = PREFIX + "CMP_U" + POSTFIX;
static {
- beforeMatchingNameRegex(CMP_U, "CmpU");
+ beforeMatchingNameRegex(CMP_U, "CmpU\\b");
}
The three existing tests currently referencing IRNode.CMP_U: compiler.c2.irTests.CmpUWithZero, compiler.intrinsics.TestCompareUnsigned, compiler.c2.irTests.TestUnsignedComparison, are all passing w/o this change. It does not break existing tests.
I'm going to push a commit to do so. If you think it's not appropriate to change IRNode.java with the scope of this issue, I can revert it.
Hi @tabjy, no worries! You're right, you cannot just append the \\b as suggested above - this was only possible in an older version of the IR framework. I think going with what you suggested with beforeMatchingNameRegex(CMP_U, "CmpU\\b") should do the trick. You can go ahead and push that. Then I can run some internal testing again, just to be sure.
@chhagedorn It's already pushed. The HEAD has the up to date master merged in. Please let me know how the test goes. Thanks!
Right, missed that. Testing is submitted, I report back once it's complete