-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8364970: Redo JDK-8327381 by updating the CmpU type instead of the Bool type #26666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This partially reverts commit a56cd37, which will no longer be needed after this change.
|
👋 Welcome back fferrari! A progress list of the required criteria for merging this PR into |
|
@franferrax This change is no longer ready for integration - check the PR body for details. |
|
@franferrax The following label will be automatically applied to this pull request:
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. |
|
@rwestrel / @TobiHartmann / @chhagedorn: this is my first contribution in C2 besides OJVG reviews and backports, please let me know if I should be testing something else. @tabjy: as the original 1383fec author, I would greatly appreciate an additional review from you. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving this! I have some small suggestions, otherwise, it looks good to me!
Additionally, having the right type in CmpUNode could potentially enable further optimizations.
Could you already find some examples, where this change gives us an improved IR? If so, you could also add it as IR test.
I'll also give this a spin in our testing.
src/hotspot/share/opto/subnode.cpp
Outdated
| // | ||
| // (1a) and (1b) is covered by this method since we can directly return the corresponding TypeInt::CC_* | ||
| // while (2) is covered in BoolNode::Ideal since we create a new non-constant node (see [CMPU_MASK]). | ||
| const Type* CmpUNode::Value_cmpu_and_mask(PhaseValues* phase, const Node* in1, const Node* in2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to directly name these:
in1 -> andI
in2- > rhs
Then it's easier to follow the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I was going to use similar names and later regretted. Suggestion accepted in 27ed1a3.
src/hotspot/share/opto/subnode.cpp
Outdated
| } | ||
|
|
||
| return _test.cc2logical(input_type); | ||
| return _test.cc2logical( phase->type( in(1) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return _test.cc2logical( phase->type( in(1) ) ); | |
| return _test.cc2logical(phase->type(in(1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion accepted in 27ed1a3.
| // | | ||
| // Bool | ||
| // | ||
| void PhaseCCP::push_bool_with_cmpu_and_mask(Unique_Node_List& worklist, const Node* use) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to double-check but I think it's fine to remove the notification code since we already have push_cmpu() in place which looks through the AddI:
jdk/src/hotspot/share/opto/phaseX.cpp
Lines 2911 to 2926 in 10762d4
| // CmpU nodes can get their type information from two nodes up in the graph (instead of from the nodes immediately | |
| // above). Make sure they are added to the worklist if nodes they depend on are updated since they could be missed | |
| // and get wrong types otherwise. | |
| void PhaseCCP::push_cmpu(Unique_Node_List& worklist, const Node* use) const { | |
| uint use_op = use->Opcode(); | |
| if (use_op == Op_AddI || use_op == Op_SubI) { | |
| for (DUIterator_Fast imax, i = use->fast_outs(imax); i < imax; i++) { | |
| Node* cmpu = use->fast_out(i); | |
| const uint cmpu_opcode = cmpu->Opcode(); | |
| if (cmpu_opcode == Op_CmpU || cmpu_opcode == Op_CmpU3) { | |
| // Got a CmpU or CmpU3 which might need the new type information from node n. | |
| push_if_not_bottom_type(worklist, cmpu); | |
| } | |
| } | |
| } | |
| } |
So, whenever m or 1 changes, we will re-add the CmpU to the CCP worklist with push_cmpu(). The x does not matter for the application of Value_cmpu_and_mask().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was oversimplifying the problem, my way of thinking it was the following one:
m x m 1
\ / \ /
AndI AddI grandparents
\ /
CmpU parent
|
Bool grandchild
"As we were updating a grandchild based on its grandparents, we needed an ad-hoc worklist push for the grandchild. Since we now update the type of CmpU based on its parents, the canonical parent-to-child propagations should work, and we don't need any ad-hoc grandparents-to-grandchild worklist push anymore."
But as you noted, non-immediate CmpU inputs such as m or 1 can change and should affect the CmpU type. Luckily, this already was the case for previous CmpU optimizations.
For case 1a, we don't need PhaseCCP::push_cmpu because m is also an immediate input of CmpU.
m x
\ /
AndI m
\ /
CmpU
|
Bool
I'm now realizing this was a very lucky situation. The AndI input isn't problematic even when PhaseCCP::push_cmpu doesn't handle the use_op == Op_AndI case, because:
xdoes not affect the application ofValue_cmpu_and_mask()- In case 1a,
mis a direct input ofCmpU - In case 1b, the
AddIinput is handled inPhaseCCP::push_cmpu(use_op == Op_AddI)
Please let me know if you think we should add a comment in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good summary! Thanks for double-checking again. It's indeed only for 1b a probably that's handled by push_cmpu(). It probably would not hurt to add a comment that push_cmpu handles this case, just to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I added the comments in 32a7940.
src/hotspot/share/opto/subnode.hpp
Outdated
| //------------------------------CmpUNode--------------------------------------- | ||
| // Compare 2 unsigned values (integer or pointer), returning condition codes (-1, 0 or 1). | ||
| class CmpUNode : public CmpNode { | ||
| static const Type* Value_cmpu_and_mask(PhaseValues*, const Node*, const Node*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually add matching parameter names as found in the source file:
static const Type* Value_cmpu_and_mask(PhaseValues* phase, const Node* in1, const Node* in2);
or with the renaming above:
static const Type* Value_cmpu_and_mask(PhaseValues* phase, const Node* andI, const Node* rhs);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion accepted in 27ed1a3.
galderz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @franferrax. Did you consider adding an IR test or similar that would expose the inconsistent state? Would it be feasible?
|
Hi @galderz, I'm afraid I had to be a bit opaque because this was partially discussed in the VG. I was just referring to the fact that the
In both cases the Since a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update looks good, thanks! I'll run some testing and report back again.
Could you already find some examples, where this change gives us an improved IR? If so, you could also add it as IR test.
Just double-checking, were you able to find such a test which now improves the IR with the better type info and CmpU while we could not with the old code? Otherwise, you could also file a follow-up RFE.
| // | | ||
| // Bool | ||
| // | ||
| void PhaseCCP::push_bool_with_cmpu_and_mask(Unique_Node_List& worklist, const Node* use) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good summary! Thanks for double-checking again. It's indeed only for 1b a probably that's handled by push_cmpu(). It probably would not hurt to add a comment that push_cmpu handles this case, just to be sure.
Sorry for not replying that, I'm working on it. We were explicitly matching the For case 1a, we were explicitly matching
For case 1b, we were explicitly matching
I will work on adding IR tests for these cases. Regarding real-world use cases, we need to rule out I've found some possible candidates but haven't fully analyzed them yet: |
|
Thank you @franferrax for catching and addressing the inconsistent state. I neglected that in my original PR. I think it would be beneficial to include your tables of the two cases in the comments too. Thank you for the hard work. |
Add a comment with the BoolTest::cc2logical inferences tables, as suggested by @tabjy. Also, add a comment explaining how PhaseCCP::push_cmpu is handling grandparent updates in the case 1b, as agreed with @chhagedorn.
According to my IGV observations, these inversions aren't necessarily effective. Also, I assume it is safe to remove them because if I apply this change to the master branch, the test still passes (tested at f95af74).
I also checked the test is now failing in the master branch (at f95af74).
@tabjy thanks for the suggestion, I added the tables in 32a7940. |
|
Hi @chhagedorn, I added the new tests in e6b1cb8. One problem I'm facing is that I'm unable to generate Even if Do you know a way to influence the This means the following 8 cases are not really testing what they claim, but repeating other cases with
Even if we don't find a way to influence the |
Correctness of the tests with the following name format should be checked in the TestFramework.run() JVM process, with the C2 compiled version of these methods. TestFramework's warmup ensures this. testCase(1a|1b)(OptimizeAsTrue|OptimizeAsFalse)For(EQ|NE|LE|GE|LT|GT)(xm|mx)
|
Learning a bit more about the IR tests framework, I noticed It should execute the compiled versions of the following But the So directly invoking I think I fixed this in e2f8a43 by making |
Absence noteToday is the last day before a ~2 weeks vacation, so my next working day is Monday, September 1st. Please feel free to keep giving feedback and/or reviews, and I will continue when I'm back. Cheers, |
src/hotspot/share/opto/subnode.cpp
Outdated
| rhs_m = rhs->in(1); | ||
| const TypeInt* rhs_m_type = phase->type(rhs_m)->isa_int(); | ||
| // Exclude any case where m == -1 is possible. | ||
| if (rhs_m_type != nullptr && (rhs_m_type->_lo > -1 || rhs_m_type->_hi < -1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use !rhs_m_type->contains(-1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted in 25aa9d7. Simple smoke-test check: builds and the IR test passes.
|
Hi @franferrax, hope you had a good vacation!
Hm, that's a good point. I was then also thinking about notification code in IGVN. We already concluded further up that it's not needed for CCP because Here are the test cases with some further comments explaining how it works and how to run it: This will produce the following IR (at |
|
Hi @chhagedorn, thank you for the additional work and your insights. This is much appreciated from a learner perspective. I didn't fully analyze the My availability will be limited as the October CPU approaches, but it will try to find some timeboxes to make |
Sure, you're welcome :-)
I think you could squeeze that in here as well. With mainline, you probably need a different notification code because we need to add the
Sounds good, no hurry. Thanks for taking another look to improve the test! |
|
@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@franferrax This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/open |
|
@franferrax This pull request is now open |
|
@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |

Hi, this pull request is a second take of 1383fec, by updating the
CmpUNodetype as eitherTypeInt::CC_LE(case 1a) orTypeInt::CC_LT(case 1b) instead of updating theBoolNodetype asTypeInt::ONE.With this approach a56cd37 becomes unnecessary. Additionally, having the right type in
CmpUNodecould potentially enable further optimizations.Testing
In order to evaluate the changes, the following testing has been performed:
jdk:tier1(see GitHub Actions run)TestBoolNodeGVN.java, created for JDK-8327381: Refactor type-improving transformations in BoolNode::Ideal to BoolNode::Value (1383fec)CmpUNode::Value_cmpu_and_maskcalltest/hotspot/jtreg/compiler/c2category on Fedora Linux x86_64master(f95af74)Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26666/head:pull/26666$ git checkout pull/26666Update a local copy of the PR:
$ git checkout pull/26666$ git pull https://git.openjdk.org/jdk.git pull/26666/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26666View PR using the GUI difftool:
$ git pr show -t 26666Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26666.diff
Using Webrev
Link to Webrev Comment