8349361: C2: RShiftL should support all applicable transformations that RShiftI does#23438
8349361: C2: RShiftL should support all applicable transformations that RShiftI does#23438rwestrel wants to merge 30 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
|
@rwestrel 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: 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 91 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
|
@rwestrel |
Webrevs
|
eme64
left a comment
There was a problem hiding this comment.
Drive-by code style comment ;)
src/hotspot/share/opto/mulnode.cpp
Outdated
| //============================================================================= | ||
| //------------------------------Identity--------------------------------------- | ||
| Node* RShiftINode::Identity(PhaseGVN* phase) { | ||
| Node *RShiftNode::IdealIL(PhaseGVN *phase, bool can_reshape, BasicType bt) { |
src/hotspot/share/opto/mulnode.cpp
Outdated
| Node *RShiftNode::IdealIL(PhaseGVN *phase, bool can_reshape, BasicType bt) { | ||
| // Inputs may be TOP if they are dead. | ||
| const TypeInteger* t1 = phase->type(in(1))->isa_integer(bt); | ||
| if (!t1) return NodeSentinel; // Left input is an integer |
There was a problem hiding this comment.
Drive-by: don't use implicit null-check, make comparison with nullptr explicit. And add curly braces.
|
Fails to build on Mac AArch64: |
Thanks for the report. Should be fixed now. I also took @eme64's comments into account. |
jaskarth
left a comment
There was a problem hiding this comment.
This is really nice! I'd wondered why there was no RShiftL::Ideal, and it's nice to have it handled it in a generic way with the integer version. I left mostly code style comments here.
| if (bt == T_INT) { | ||
| return BitsPerJavaInteger; | ||
| } | ||
| return BitsPerJavaLong; |
There was a problem hiding this comment.
I think it'd be nice to add assert(bt == T_LONG, "unsupported"); before the last return, like in the helper methods above.
There was a problem hiding this comment.
Indeed. Added in new commit.
src/hotspot/share/opto/mulnode.cpp
Outdated
| jlong hi = r1->hi_as_long() >> (jint)shift; | ||
| assert(lo <= hi, "must have valid bounds"); | ||
| #ifdef ASSERT | ||
| if (bt ==T_INT) { |
There was a problem hiding this comment.
| if (bt ==T_INT) { | |
| if (bt == T_INT) { |
Could this assert be generic to also handle T_LONG too?
There was a problem hiding this comment.
The assert checks that, for the int case:
long lo;
assert((int)(lo >> shift) == (((int)lo) >> shift, "");
For long, it would be:
long lo;
assert((long)(lo >> shift) == (((long)lo) >> shift, "");
Given everything is already a long, that's:
long lo;
assert(lo >> shift == lo >> shift, "");
There was a problem hiding this comment.
Ah I see, thank you for the explanation!
Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com>
Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com>
Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com>
Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com>
Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com>
Co-authored-by: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com>
Thanks for reviewing this. I pushed a new commit that takes your comments into account. |
jaskarth
left a comment
There was a problem hiding this comment.
Thanks for the update, it looks good!
eme64
left a comment
There was a problem hiding this comment.
@rwestrel nice work, looks like a good step to unify the code a little!
I left some comments / suggestions.
I'm also wondering about testing. How good do you think test coverage is? Are all cases covered? How about the edge-cases? Could we improve the coverage with randomization somehow?
src/hotspot/share/opto/mulnode.cpp
Outdated
| if (t1 == nullptr) { | ||
| return NodeSentinel; // Left input is an integer | ||
| } | ||
| const TypeInteger* t3; // type of in(1).in(2) |
There was a problem hiding this comment.
I know that you only moved this code, but it looks bad 🙈
For one, why is it defined up here already when it is only used 10 lines later?
And why not give it a better name so we don't need the comment?
| const TypeInteger* t3; // type of in(1).in(2) |
src/hotspot/share/opto/mulnode.cpp
Outdated
| (t3 = phase->type(mask->in(2))->isa_integer(bt)) && | ||
| t3->is_con()) { | ||
| jlong maskbits = t3->get_con_as_long(bt); | ||
| // Convert to "(x >> shift) & (mask >> shift)" |
There was a problem hiding this comment.
This is a nice comment. It could come as a motivation above. Because it suggests that we can then constant fold the mask >> shift, right?
src/hotspot/share/opto/mulnode.cpp
Outdated
| const Node* mask = in(1); | ||
| if (mask->Opcode() == Op_And(bt) && | ||
| (t3 = phase->type(mask->in(2))->isa_integer(bt)) && | ||
| t3->is_con()) { | ||
| jlong maskbits = t3->get_con_as_long(bt); |
There was a problem hiding this comment.
This is also quite bad. It seems mask here is in(1), which is not even the mask at all, but x & <real_mask>.
I'd suggest to clean it up a little and use better names.
src/hotspot/share/opto/mulnode.cpp
Outdated
| if (progress != nullptr) { | ||
| return progress; | ||
| } | ||
| const TypeInt* t3; // type of in(1).in(2) |
There was a problem hiding this comment.
Also refactor the use of t3 here, please.
| } | ||
|
|
||
| @Run(test = { "test1", "test2", "test3", "test4" }) | ||
| @Run(test = { "test1", "test2", "test3", "test4", "test5", "test6", "test7", "test8", "test9" }) |
There was a problem hiding this comment.
You should add the bug id above.
| final int test7Shift = 42; | ||
| final long test7Min = -1L << (64 - test7Shift -1); | ||
| final long test7Max = ~test7Min; |
There was a problem hiding this comment.
Could we randomize these tests, so that we would get better coverage?
Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
Thanks for reviewing this. I pushed new commits that I think cover your comments.
Transformations that I refactored and now apply to both the long and int |
|
@eme64 can you take another look? |
Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
…Tests.java Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
chhagedorn
left a comment
There was a problem hiding this comment.
Nice refactoring! I have a few small comments - mostly code style. Otherwise, looks good to me, too.
src/hotspot/share/opto/mulnode.cpp
Outdated
| if (t1 == Type::TOP) return Type::TOP; | ||
| if (t2 == Type::TOP) return Type::TOP; | ||
|
|
||
| // Left input is ZERO ==> the result is ZERO. | ||
| if( t1 == TypeInt::ZERO ) return TypeInt::ZERO; | ||
| if (t1 == TypeInteger::zero(bt)) return TypeInteger::zero(bt); | ||
| // Shift by zero does nothing | ||
| if( t2 == TypeInt::ZERO ) return t1; | ||
| if (t2 == TypeInt::ZERO) return t1; |
There was a problem hiding this comment.
Can you add braces here for safety?
src/hotspot/share/opto/mulnode.cpp
Outdated
| int hi = ~lo; // 00007FFF | ||
| const TypeInt* t11 = phase->type(in(1)->in(1))->isa_int(); | ||
| jlong lo = (-1 << (bits_per_java_integer(bt) - ((uint)count)-1)); // FFFF8000 | ||
| jlong hi = ~lo; // 00007FFF |
There was a problem hiding this comment.
Seems strangely aligned. Maybe either align it to the comment above or convert to an unaligned comment.
| if (in(1)->Opcode() == Op_LShift(bt) && | ||
| in(1)->req() == 3 && | ||
| in(1)->in(2) == in(2)) { |
There was a problem hiding this comment.
Generally, is there notifaction code for this pattern to re-add the node to the IGVN worklist? If not, I don't think you need to handle it here if it's missing (it's just a missed opportunity but no correctness issue) but would be good to file a follow-up bug to handle it - especially when we want to add IGVN verification for Ideal and Identity with JDK-8347273.
There was a problem hiding this comment.
Good catch. I fixed a case that was handled for int but not for long. There are others that are missing for both AFAICT.
If I file a follow up bug, writing a test case for it is going to be very hard. So testing a fix is also hard. Shouldn't we wait for JDK-8347273 and fix whatever follows up of that?
There was a problem hiding this comment.
If I file a follow up bug, writing a test case for it is going to be very hard
Yes, it will be. There is currently no way to really verify that reliably without the additional verification. I think it's okay to wait for JDK-8347273. Maybe you can add a note there or file a separate issue to keep track of the missing bits detected in this investigation..
src/hotspot/share/opto/type.cpp
Outdated
| return TypeLong::make(lo, hi, w); | ||
| } | ||
|
|
||
| const TypeInteger* TypeInteger::make(jlong lo, BasicType bt) { |
There was a problem hiding this comment.
Maybe you want to rename lo to con since we set lo == hi.
| @@ -27,7 +27,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
Up to you if you want to update the copyright year or add your company's copyright. Same in the other test.
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Thanks for the review. |
|
@rwestrel I saw this in testing from yesterday: |
|
Not sure if that still reproduces after your changes. LMK when I should run testing again. |
chhagedorn
left a comment
There was a problem hiding this comment.
Apart from Emanuel's report, it looks good to me, thanks for the update!
I could reproduce that one by forcing one of the random values to a particular constant. It's a bug in the test that was fixed since. @eme64 could you run testing again, please? |
|
@eme64 any update on testing? |
|
@rwestrel Testing looks good, thanks for the ping :) |
|
@eme64 thanks for running tests and for the review. |
|
/integrate |
|
Going to push as commit 79bffe2.
Your commit was automatically rebased without conflicts. |
This change refactors
RShiftI/RshiftLIdeal,IdentityandValuebecause theintandlongversions are very similar and sothere's no logic duplication. In the process, support for some extra
transformations is added to
RShiftL. I also added some new testcases.
/cc hotspot-compiler
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23438/head:pull/23438$ git checkout pull/23438Update a local copy of the PR:
$ git checkout pull/23438$ git pull https://git.openjdk.org/jdk.git pull/23438/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23438View PR using the GUI difftool:
$ git pr show -t 23438Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23438.diff
Using Webrev
Link to Webrev Comment