8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value #18198

tabjy · 2024-03-11T14:58:06Z

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

Change must not contain extraneous whitespace
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) Review applies to e2eb8bf9
Emanuel Peter (@eme64 - Reviewer) Review applies to 84784c74
Christian Hagedorn (@chhagedorn - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

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

Link to Webrev Comment

bridgekeeper · 2024-03-11T16:36:37Z

👋 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.

openjdk · 2024-03-11T16:39:10Z

@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.

mlbridge · 2024-03-11T16:42:16Z

Webrevs

also excluding negative values for unsigned comparison

jaskarth

I think the cleanup looks good! I have mostly stylistic suggestions here. Also, the copyright header in subnode.cpp should be updated to read 2024.

src/hotspot/share/opto/subnode.cpp

test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java

eme64

Looks like a reasonable idea. Running tests now. Will review afterwards.

src/hotspot/share/opto/subnode.cpp

tabjy · 2024-03-12T18:40:48Z

Thanks @jaskarth and @eme64 for the review. I've pushed a new commit to address the following:

Updated license header year to 2024
Explicit nullptr comparison
Node* var for pointer types
Test moved to c2.irTests, added @bug and @summary tags

jaskarth

Thanks for the update! Just one more thing from me.

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java

tabjy · 2024-03-13T02:02:34Z

Oops. Package name updated. Sorry for such a rookie mistake!

jaskarth

No worries, looks good to me now :)

openjdk · 2024-03-13T20:11:03Z

@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:

32c9750: 8339160: [BACKOUT] JDK-8338440 Parallel: Improve fragmentation mitigation in Full GC
9d183bd: 8339149: jfr_flush_event_writer - return value type mismatch
2150521: 8322036: Improve help output from the javadoc tool
1ff9ac7: 8338731: MemoryLayout::offsetHandle can return a negative offset
2e174c6: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest
8e88da0: 8338041: Keyboard Navigation of JTable, Ctrl Shift RIGHT/LEFT doesn't follow native action in GTK L&F
449ca2c: 8337832: Optimize datetime toString
b1b4cd4: 8332158: [XWayland] test/jdk/java/awt/Mouse/EnterExitEvents/ResizingFrameTest.java
284c3cd: 8336299: Improve GCLocker stall diagnostics
2e96f15: 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).

merykitty · 2024-03-18T04:36:18Z

(x & m) u< m + 1 is false for m = -1, right?

Edit: Yep, filed JDK-8328315.

jaskarth · 2024-03-19T05:08:53Z

Ah, I had assumed the transformation was valid beforehand, since it had existed for a while 😅 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.

merykitty · 2024-03-19T13:45:19Z

@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 · 2024-03-19T16:16:49Z

@tabjy I am re-running testing, then will re-review.
/reviewers 2 reviewer

openjdk · 2024-03-19T16:17:43Z

@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).

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java

eme64

The cpp code looks good, I have some small issues with the test. But we are close ;)

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java

…P_U nodes

tabjy · 2024-07-08T16:46:33Z

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)

TobiHartmann · 2024-07-22T06:32:39Z

Thanks @tabjy! @chhagedorn, our IR Framework expert, is currently on vacation but will be back later this week.

eme64 · 2024-07-22T06:47:33Z

@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 · 2024-07-24T10:50:38Z

@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:

jdk/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java

Lines 495 to 499 in 05d88de

    
           public static final String COUNTED_LOOP = PREFIX + "COUNTED_LOOP" + POSTFIX; 
        
           static { 
        
               String regex = START + "CountedLoop\\b" + MID + END; 
        
               fromAfterCountedLoops(COUNTED_LOOP, regex); 
        
           }

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);
    }

eme64 · 2024-08-15T06:39:20Z

@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.

chhagedorn · 2024-08-22T05:59:55Z

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.

tabjy · 2024-08-26T17:30:19Z

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.

jdk/test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java

Lines 440 to 443 in a15af69

    
           public static final String CMP_U = PREFIX + "CMP_U" + POSTFIX; 
        
           static { 
        
               beforeMatchingNameRegex(CMP_U, "CmpU"); 
        
           }

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.

chhagedorn · 2024-08-27T07:08:25Z

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.

tabjy · 2024-08-27T07:12:50Z

@chhagedorn It's already pushed. The HEAD has the up to date master merged in. Please let me know how the test goes. Thanks!

chhagedorn · 2024-08-27T07:36:24Z

Right, missed that. Testing is submitted, I report back once it's complete

chhagedorn

Testing looked good!

TobiHartmann

That looks good to me. @eme64 is currently out but you can integrate this now.

tabjy · 2024-08-28T14:09:55Z

/integrate

openjdk · 2024-08-28T14:12:01Z

@tabjy
Your change (at version 719199c) is now ready to be sponsored by a Committer.

TobiHartmann · 2024-08-29T05:32:36Z

/sponsor

openjdk · 2024-08-29T05:34:08Z

Going to push as commit 1383fec.
Since your change was applied there have been 41 commits pushed to the master branch:

eb7ead5: 8336873: BasicSplitPaneDivider:oneTouchExpandableChanged() should mention that implementation depends on SplitPane.supportsOneTouchButtons property
0ddcd70: 8335120: assert(!target->can_be_statically_bound() || target == cha_monomorphic_target) failed
26e3d53: 8338716: Re-visit "interrupt handling" in jdk.internal.loader.Resource
72a4900: 8338888: SystemDictionary::class_name_symbol has incorrect length check
a8ac287: 8339126: JNI exception pending in Inflater.c
d08b5bd: 8258483: [TESTBUG] gtest CollectorPolicy.young_scaled_initial_ergo_vm fails if heap is too small
d03ec7a: 8339030: frame::print_value_on(outputStream* st, JavaThread *thread) doesn't need thread argument
eff6d9c: 8339167: Remove AbstractPoolEntry.PrimitiveEntry to reduce boxing overheads
a98ecad: 8338897: Small startup regression remains after JDK-8309622 and JDK-8331932
3d49fb8: 8338103: Stabilize and open source a Swing OGL ButtonResizeTest
... and 31 more: https://git.openjdk.org/jdk/compare/a15af6998e8f7adac2ded94ef5a47e22ddb53452...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-29T05:34:16Z

@TobiHartmann @tabjy Pushed as commit 1383fec.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

tabjy added 4 commits February 6, 2024 12:48

refactor (x & m) u<= m transformation and add test

8af8bd6

exclude x86 from tests

1359237

also test for correctness

6471129

add license header

aa7fafb

openjdk bot added the rfr Pull request is ready for review label Mar 11, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 11, 2024

fix test by adding the missing inversion

17a9dc3

also excluding negative values for unsigned comparison

jaskarth reviewed Mar 11, 2024

View reviewed changes

src/hotspot/share/opto/subnode.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/subnode.cpp Outdated Show resolved Hide resolved

test/hotspot/jtreg/compiler/c2/TestBoolNodeGvn.java Outdated Show resolved Hide resolved

eme64 suggested changes Mar 12, 2024

View reviewed changes

src/hotspot/share/opto/subnode.cpp Outdated Show resolved Hide resolved

modification per code review suggestions

06b7da3

jaskarth reviewed Mar 12, 2024

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java Outdated Show resolved Hide resolved

update the package name for tests

e2eb8bf

jaskarth approved these changes Mar 13, 2024

View reviewed changes

tabjy requested a review from eme64 March 18, 2024 01:38

eme64 reviewed Mar 20, 2024

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java Outdated Show resolved Hide resolved

eme64 suggested changes Mar 20, 2024

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java Outdated Show resolved Hide resolved

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java Outdated Show resolved Hide resolved

test/hotspot/jtreg/compiler/c2/irTests/TestBoolNodeGvn.java Outdated Show resolved Hide resolved

tabjy added 3 commits March 25, 2024 12:47

Merge branch 'master' into boolnode-refactor

3bb431c

improve formatting, correct annotation and rename test class

4867958

update test @run annotation

47d0172

spread boolean AND and OR into subcases, update number of expected CM…

b731f6d

…P_U nodes

Merge branch 'master' into boolnode-refactor

29655d3

tabjy mentioned this pull request Jul 22, 2024

8328528: C2 should optimize long-typed parallel iv in an int counted loop #18489

Closed

3 tasks

tabjy added 2 commits August 26, 2024 13:27

add a word break to IRNode.CMP_U

b0924fd

Merge branch 'master' into boolnode-refactor

719199c

chhagedorn approved these changes Aug 27, 2024

View reviewed changes

tabjy requested a review from eme64 August 27, 2024 15:21

TobiHartmann approved these changes Aug 28, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 28, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 28, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 29, 2024

openjdk bot closed this Aug 29, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 29, 2024

8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value #18198

8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value #18198

Uh oh!

Conversation

tabjy commented Mar 11, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 11, 2024

Uh oh!

openjdk bot commented Mar 11, 2024

Uh oh!

mlbridge bot commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jaskarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tabjy commented Mar 12, 2024

Uh oh!

jaskarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tabjy commented Mar 13, 2024

Uh oh!

jaskarth left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merykitty commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaskarth commented Mar 19, 2024

Uh oh!

merykitty commented Mar 19, 2024

Uh oh!

eme64 commented Mar 19, 2024

Uh oh!

openjdk bot commented Mar 19, 2024

Uh oh!

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tabjy commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann commented Jul 22, 2024

Uh oh!

eme64 commented Jul 22, 2024

Uh oh!

chhagedorn commented Jul 24, 2024

Uh oh!

eme64 commented Aug 15, 2024

Uh oh!

chhagedorn commented Aug 22, 2024

Uh oh!

tabjy commented Aug 26, 2024

Uh oh!

chhagedorn commented Aug 27, 2024

Uh oh!

tabjy commented Aug 27, 2024

tabjy commented Mar 11, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 11, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading

merykitty commented Mar 18, 2024 •

edited

Loading

tabjy commented Jul 8, 2024 •

edited

Loading