8302814: Delete unused CountLoopEnd instruct with CmpX #12648

sunny868 · 2023-02-20T07:34:08Z

CountLoopEnd only for T_int, therefore the following instructs in riscv.ad are useless and should be deleted.

CountedLoopEnd cmp (CmpU op1 op2)
CountedLoopEnd cmp (CmpP op1 op2)
CountedLoopEnd cmp (CmpN op1 op2)
CountedLoopEnd cmp (CmpF op1 op2)
CountedLoopEnd cmp (CmpD op1 op2)

and CountedLoopEnd with CmpU on x86*.ad, aarch64.ad ar useless also.

Please help review it.

Thanks.

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 1 Reviewer, 1 Author)

Issue

JDK-8302814: Delete unused CountLoopEnd instruct with CmpX

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to fefb9122
Feilong Jiang (@feilongjiang - Author) ⚠️ Review applies to fefb9122

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12648/head:pull/12648
$ git checkout pull/12648

Update a local copy of the PR:
$ git checkout pull/12648
$ git pull https://git.openjdk.org/jdk.git pull/12648/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12648

View PR using the GUI difftool:
$ git pr show -t 12648

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12648.diff

bridgekeeper · 2023-02-20T07:35:21Z

👋 Welcome back sunny868! 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 · 2023-02-20T07:37:16Z

@sunny868 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 · 2023-02-20T07:41:12Z

Webrevs

sunny868 · 2023-02-21T01:26:19Z

The failure linux-cross-compile / build (arm) is not related to this patch.

feilongjiang · 2023-02-21T02:29:50Z

Hi, did you run any tests on these changes? I agree with removing CmpP/CmpN/CmpF/CmpD instructs. But for CmpU, I have checked aarch64.ad, and it provides the U version of CountedLoopEnd, I think we should do some investigating into this:

jdk/src/hotspot/cpu/aarch64/aarch64.ad

Lines 16526 to 16541 in 43c71dd

    
           // counted loop end branch near Unsigned 
        
           instruct branchLoopEndU(cmpOpU cmp, rFlagsRegU cr, label lbl) 
        
           %{ 
        
             match(CountedLoopEnd cmp cr); 
        
             effect(USE lbl); 
        
             ins_cost(BRANCH_COST); 
        
             // short variant. 
        
             // ins_short_branch(1); 
        
             format %{ "b$cmp $lbl \t// counted loop end unsigned" %} 
        
             ins_encode(aarch64_enc_br_conU(cmp, lbl)); 
        
             ins_pipe(pipe_branch); 
        
           %}

And I also found LongCountedLoopEndNode definition in loopnode.hpp but not used yet:

jdk/src/hotspot/share/opto/loopnode.hpp

Lines 356 to 371 in 43c71dd

    
           class LongCountedLoopNode : public BaseCountedLoopNode { 
        
           public: 
        
             LongCountedLoopNode(Node *entry, Node *backedge) 
        
               : BaseCountedLoopNode(entry, backedge) { 
        
               init_class_id(Class_LongCountedLoop); 
        
             } 
        
             virtual int Opcode() const; 
        
             virtual BasicType bt() const { 
        
               return T_LONG; 
        
             } 
        
             LongCountedLoopEndNode* loopexit_or_null() const { return (LongCountedLoopEndNode*) BaseCountedLoopNode::loopexit_or_null(); } 
        
             LongCountedLoopEndNode* loopexit() const { return (LongCountedLoopEndNode*) BaseCountedLoopNode::loopexit(); } 
        
           };

Maybe we could keep the Long version?

sunny868 · 2023-02-22T01:18:40Z

I only did tier1-3 and openjvm2008 tests on the LoongArch64 architecture(Linux platform) and confirmed that none of these types were used, including CmpU.
Do you sure the U version of CountedLoopEnd be used on AArch64? I guess it is unused also but I will also confirm further.
Yes, I will update the Patch for keep the Long version. I'm also evaluating the performance benefits of LongCountedLoopEndNode to decide whether to implement it.

feilongjiang · 2023-02-22T03:09:39Z

Do you sure the U version of CountedLoopEnd be used on AArch64?

I'm not sure about that. I just found that CmpU version was defined in aarch64.ad too.

I guess it is unused also but I will also confirm further.

Yes, we should check if CmpU was safe to remove. By the way, looks x86 also provides CmpU version of CountedeLoopEnd:

jdk/src/hotspot/cpu/x86/x86_64.ad

Lines 13056 to 13069 in 180b94c

    
           // Jump Direct Conditional - Label defines a relative address from Jcc+1 
        
           instruct jmpLoopEndU(cmpOpU cop, rFlagsRegU cmp, label labl) %{ 
        
             match(CountedLoopEnd cop cmp); 
        
             effect(USE labl); 
        
             ins_cost(300); 
        
             format %{ "j$cop,u   $labl\t# loop end" %} 
        
             size(6); 
        
             ins_encode %{ 
        
               Label* L = $labl$$label; 
        
               __ jcc((Assembler::Condition)($cop$$cmpcode), *L, false); // Always long jump 
        
             %} 
        
             ins_pipe(pipe_jcc); 
        
           %}

sunny868 · 2023-02-23T09:57:30Z

For bytecodes if_icmpxx(used by javas for loop)、ifeq/ifne/iflt/ifle/ifgt/ifge, if_icmpxx(used by javas while loop), C2 compiler created CmpINode not CmpUNode, so I think that should doesn't exist CountedLoopNode with CmpUNode.


// src/hotspot/share/opto/parse2.cpp

2661   case Bytecodes::_ifeq: btest = BoolTest::eq; goto handle_ifxx;                
2662   case Bytecodes::_ifne: btest = BoolTest::ne; goto handle_ifxx;                
2663   case Bytecodes::_iflt: btest = BoolTest::lt; goto handle_ifxx;                
2664   case Bytecodes::_ifle: btest = BoolTest::le; goto handle_ifxx;                
2665   case Bytecodes::_ifgt: btest = BoolTest::gt; goto handle_ifxx;                
2666   case Bytecodes::_ifge: btest = BoolTest::ge; goto handle_ifxx;                
2667   handle_ifxx:                                                                  
2668     // If this is a backwards branch in the bytecodes, add Safepoint            
2669     maybe_add_safepoint(iter().get_dest());                                     
2670     a = _gvn.intcon(0);                                                         
2671     b = pop();                                                                  
2672     c = _gvn.transform( new CmpINode(b, a) );                                   
2673     do_if(btest, c);                                                            
2674     break;                                                                      
2675                                                                                 
2676   case Bytecodes::_if_icmpeq: btest = BoolTest::eq; goto handle_if_icmp;        
2677   case Bytecodes::_if_icmpne: btest = BoolTest::ne; goto handle_if_icmp;        
2678   case Bytecodes::_if_icmplt: btest = BoolTest::lt; goto handle_if_icmp;        
2679   case Bytecodes::_if_icmple: btest = BoolTest::le; goto handle_if_icmp;        
2680   case Bytecodes::_if_icmpgt: btest = BoolTest::gt; goto handle_if_icmp;        
2681   case Bytecodes::_if_icmpge: btest = BoolTest::ge; goto handle_if_icmp;        
2682   handle_if_icmp:                                                               
2683     // If this is a backwards branch in the bytecodes, add Safepoint            
2684     maybe_add_safepoint(iter().get_dest());                                     
2685     a = pop();                                                                  
2686     b = pop();                                                                  
2687     c = _gvn.transform( new CmpINode( b, a ) );

vnkozlov

What about jmpLoopEndUCF* instructions?

vnkozlov

Also I think it is not correct to remove CmpU case. There are IGVN transformations which convert CmpI to CmpU:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/subnode.cpp#L1632

May be that is why next test failed in your GHA testing:
compiler/vectorization/runner/LoopRangeStrideTest.java

vnkozlov · 2023-02-23T18:39:39Z

I am taking back my next comment because I see this test fails in other PRs:

May be that is why next test failed in your GHA testing:
compiler/vectorization/runner/LoopRangeStrideTest.java

sunny868 · 2023-02-24T05:27:55Z

Also I think it is not correct to remove CmpU case. There are IGVN transformations which convert CmpI to CmpU: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/subnode.cpp#L1632

Maybe it only for range check. see:

Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x1867734] BoolNode::Ideal(PhaseGVN*, bool)+0xb84 (subnode.cpp:1631)
V [libjvm.so+0x1567f65] PhaseGVN::transform_no_reclaim(Node*)+0x25 (phaseX.cpp:840)
V [libjvm.so+0x124b4f5] LibraryCallKit::generate_limit_guard(Node*, Node*, Node*, RegionNode*)+0x145 (library_call.cpp:891)
V [libjvm.so+0x122085a] LibraryCallKit::generate_string_range_check(Node*, Node*, Node*, bool)+0xea (library_call.cpp:912)
V [libjvm.so+0x12340c3] LibraryCallKit::inline_string_copy(bool)+0x203 (library_call.cpp:1403)
V [libjvm.so+0x1248ee4] LibraryIntrinsic::generate(JVMState*)+0x3d4 (library_call.cpp:116)

sunny868 · 2023-02-24T05:28:16Z

What about jmpLoopEndUCF* instructions?

Thanks @vnkozlov for the reminder, I think jmpLoopEndUCF* is unused also.

vnkozlov

I am not convinced about CmpU but I submitted testing for v03 to see results.

vnkozlov · 2023-02-24T19:06:57Z

/reviewers 2

vnkozlov

My tier1-4, xcomp and stress testing passed. I can't complain now :)
You need second review.

openjdk · 2023-02-24T19:09:06Z

⚠️ @sunny868 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8302814
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2023-02-24T19:09:08Z

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

8302814: Delete unused CountLoopEnd instruct with CmpX

Reviewed-by: kvn, fjiang

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@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).

openjdk · 2023-02-24T19:09:37Z

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

feilongjiang · 2023-02-27T03:32:09Z

src/hotspot/cpu/riscv/riscv.ad

@@ -9294,25 +9160,6 @@ instruct cmpL_reg_imm0_branch(cmpOp cmp, iRegL op1, immL0 zero, label lbl)
  ins_short_branch(1);
 %}

-instruct cmpL_reg_imm0_loop(cmpOp cmp, iRegL op1, immL0 zero, label lbl)


I think this instruction should keep too.

The current count loop iterations is1000(setting by LoopStripMiningIter ), and if it is too large, it will cause the GC pause too long, so I don't think there is any point in implementing the counted loop for Long type. However, I still respect your opinion and keep cmpL loop in risv.ad.

feilongjiang

New changes look good, thanks.

vnkozlov

v04 update is good

sunny868 · 2023-02-28T00:11:48Z

/integrate

openjdk · 2023-02-28T00:13:48Z

@sunny868
Your change (at version fefb912) is now ready to be sponsored by a Committer.

sunny868 · 2023-02-28T00:16:45Z

Thanks @vnkozlov @feilongjiang for review.

bridgekeeper · 2023-03-28T05:49:11Z

@sunny868 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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

sunny868 · 2023-03-28T06:31:51Z

@vnkozlov Do you have any comments on this patch and could you please sponsor it for me?

vnkozlov · 2023-03-28T23:44:05Z

@sunny868 please merge master and I will retest it.

And please ask for sponsorship sooner. I did not see you needed it.

sunny868 · 2023-03-29T01:14:02Z

/integrate

openjdk · 2023-03-29T01:14:59Z

@sunny868
Your change (at version 0652b11) is now ready to be sponsored by a Committer.

vnkozlov · 2023-03-29T17:17:33Z

My testing (tier1-4, xcomp, stress) passed with latest version.

vnkozlov · 2023-03-29T17:17:47Z

/sponsor

openjdk · 2023-03-29T17:19:05Z

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

34f4d7f: 8304759: Add BitMap iterators
42df1a9: 8304991: Redundant hyphen in @param results in double-dash in javadocs
e3855d0: 8304840: Dangling CharacterCodingException in a few javadoc descriptions
6727490: 8303161: [vectorapi] VectorMask.cast narrow operation returns incorrect value with SVE
f07decb: 8272613: CharsetDecoder.decode(ByteBuffer) throws IllegalArgumentException
014c658: 8305086: G1 Redirty Cards phase printed twice
2fa0933: 8304990: unnecessary dash in @param gives double-dash in docs
e56bcb0: 8305095: Update java/net/httpclient/CustomRequestPublisher.java to use new HttpTestServer factory methods
0985288: 8304681: compiler/sharedstubs/SharedStubToInterpTest.java fails after JDK-8304387
ff368d5: 8304867: Explicitly disable dtrace for ppc builds
... and 1 more: https://git.openjdk.org/jdk/compare/7239150f8aff0e3dc07c5b27f6b7fb07237bfc55...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-29T17:19:20Z

@vnkozlov @sunny868 Pushed as commit be764a7.

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

sunny868 and others added 2 commits February 18, 2023 15:58

8302814: Delete unused CountLoopEnd instruct with CmpX

e266a83

Merge branch 'openjdk:master' into 8302814

f1fe81d

openjdk bot added the rfr Pull request is ready for review label Feb 20, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 20, 2023

sunny868 changed the title ~~8301814: RISCV: Delete unused CountLoopEnd instruct with CmpX~~ 8302814: Delete unused CountLoopEnd instruct with CmpX Feb 23, 2023

sunny868 and others added 2 commits February 23, 2023 16:27

Merge branch 'openjdk:master' into 8302814

773826e

8302814: Delete unused CountLoopEnd instruct with CmpX

d2aa86e

vnkozlov reviewed Feb 23, 2023

View reviewed changes

vnkozlov suggested changes Feb 23, 2023

View reviewed changes

8302814: Delete unused CountLoopEnd instruct with CmpX

1c89175

vnkozlov reviewed Feb 24, 2023

View reviewed changes

vnkozlov approved these changes Feb 24, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 24, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Feb 24, 2023

feilongjiang suggested changes Feb 27, 2023

View reviewed changes

8302814: Delete unused CountLoopEnd instruct with CmpX

fefb912

feilongjiang approved these changes Feb 27, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 27, 2023

vnkozlov approved these changes Feb 27, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 28, 2023

Merge branch 'openjdk:master' into 8302814

0652b11

openjdk bot removed the sponsor Pull request is ready to be sponsored label Mar 29, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 29, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 29, 2023

openjdk bot closed this Mar 29, 2023

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 Mar 29, 2023

sunny868 deleted the 8302814 branch March 30, 2023 02:48

8302814: Delete unused CountLoopEnd instruct with CmpX #12648

8302814: Delete unused CountLoopEnd instruct with CmpX #12648

Conversation

sunny868 commented Feb 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

mlbridge bot commented Feb 20, 2023 • edited Loading

Webrevs

sunny868 commented Feb 21, 2023

feilongjiang commented Feb 21, 2023

sunny868 commented Feb 22, 2023

feilongjiang commented Feb 22, 2023

sunny868 commented Feb 23, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov commented Feb 23, 2023

sunny868 commented Feb 24, 2023

sunny868 commented Feb 24, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov commented Feb 24, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 24, 2023

openjdk bot commented Feb 24, 2023 • edited Loading

openjdk bot commented Feb 24, 2023

feilongjiang Feb 27, 2023

Choose a reason for hiding this comment

sunny868 Feb 27, 2023

Choose a reason for hiding this comment

feilongjiang left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

sunny868 commented Feb 28, 2023

openjdk bot commented Feb 28, 2023

sunny868 commented Feb 28, 2023

bridgekeeper bot commented Mar 28, 2023

sunny868 commented Mar 28, 2023

vnkozlov commented Mar 28, 2023 • edited Loading

sunny868 commented Mar 29, 2023

openjdk bot commented Mar 29, 2023

vnkozlov commented Mar 29, 2023

vnkozlov commented Mar 29, 2023

openjdk bot commented Mar 29, 2023

openjdk bot commented Mar 29, 2023

sunny868 commented Feb 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 20, 2023 •

edited

Loading

openjdk bot commented Feb 24, 2023 •

edited

Loading

vnkozlov commented Mar 28, 2023 •

edited

Loading