RFR: 8336759: C2: int counted loop with long limit not recognized as counted loop

Tue Dec 3 09:40:44 UTC 2024

On Fri, 29 Nov 2024 01:08:04 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

> This patch implements [JDK-8336759](https://bugs.openjdk.org/browse/JDK-8336759) that recognizes int counted loops with long limits.
> 
> Currently, patterns like `for ( int i =...; i < long_limit; ...)` where int `i` is implicitly promoted to long (i.e., `(long) i < long_limit`) is not recognized as (int) counted loop. This patch speculatively and optimistically converts long limits to ints and deoptimize if the limit is outside int range, allowing more optimization opportunities. 
> 
> In other words, it transforms 
> 
> 
> for (int i = 0; (long) i < long_limit; i++) {...}
> 
> 
> to 
> 
> 
> if (int_min <= long_limit && long_limit <= int_max ) {
>     for (int i = 0;  i < (int) long_limit; i++) {...}
> } else {
>     trap: loop_limit_check
> }
> 
> 
> This could benefit calls to APIs like `long MemorySegment#byteSize()` when iterating over a long limit.

Nice enhancement! Couple of mostly minor comments.

src/hotspot/share/opto/loopnode.cpp line 1625:

> 1623: // counted loop but deoptimize if the limit is out of int range.
> 1624: // This is common pattern with "for (int i =...; i < long_limit; ...)" where int "i" is implicitly promoted to long,
> 1625: // i.e., "(long) i < long_limit", and therefore making it not an int counted loop without this transformation.

Maybe add here the visualization from the PR which makes it easier to grasp, something like:

In summary, we transform

  for (int i = 0; (long) i < long_limit; i++) {...}

to

  if (int_min <= long_limit && long_limit <= int_max ) {
    for (int i = 0;  i < (int) long_limit; i++) {...}
  } else {
    trap: loop_limit_check
  }

src/hotspot/share/opto/loopnode.cpp line 1626:

> 1624: // This is common pattern with "for (int i =...; i < long_limit; ...)" where int "i" is implicitly promoted to long,
> 1625: // i.e., "(long) i < long_limit", and therefore making it not an int counted loop without this transformation.
> 1626: bool PhaseIdealLoop::is_counted_loop_with_speculative_long_limit(Node* x, IdealLoopTree*&loop, BasicType iv_bt) {

I suggest to rename `x` to `loop_head` (could also be done in `do_is_counted_loop()`):
Suggestion:

bool PhaseIdealLoop::is_counted_loop_with_speculative_long_limit(Node* loop_head, IdealLoopTree*&loop, BasicType iv_bt) {

src/hotspot/share/opto/loopnode.cpp line 1638:

> 1636:   Node* init_control = x->in(LoopNode::EntryControl);
> 1637: 
> 1638:   // Make sure there is a parse predicate for us to insert the loop limit check.

I usually write the predicate names capitalized to better highlight that these are well-defined names:
Suggestion:

  // Make sure there is a Loop Limit Check Parse Predicate for us to insert the Loop Limit Check Predicate above it.

src/hotspot/share/opto/loopnode.cpp line 1642:

> 1640:   const PredicateBlock* loop_limit_check_predicate_block = predicates.loop_limit_check_predicate_block();
> 1641:   if (!loop_limit_check_predicate_block->has_parse_predicate()) {
> 1642:     return false;

I suggest to also add here a trace message, similar to what we already do in `is_counted_loop()`:
https://github.com/openjdk/jdk/blob/c330b90b9f43f80c322153585fa78704358f0224/src/hotspot/share/opto/loopnode.cpp#L2013-L2023

src/hotspot/share/opto/loopnode.cpp line 1671:

> 1669:       cmp->in(1) == incr
> 1670:           ? new CmpINode(new_incr, new_limit) : new CmpINode(new_limit, new_incr),
> 1671:       cmp);

This is a little hard to read. How about:
Suggestion:

  Node* new_cmp = cmp->in(1) == incr ? new CmpINode(new_incr, new_limit) : new CmpINode(new_limit, new_incr);
  _igvn.register_new_node_with_optimizer(new_cmp, cmp);

src/hotspot/share/opto/loopnode.cpp line 1684:

> 1682:     _igvn.rehash_node_delayed(bol);
> 1683:     bol->replace_edge(new_cmp, cmp, &_igvn);
> 1684: 

New line can be removed
Suggestion:

src/hotspot/share/opto/loopnode.cpp line 1696:

> 1694:   Node* bol_limit = new BoolNode(cmp_limit, BoolTest::eq);
> 1695:   insert_loop_limit_check_predicate(init_control->as_IfTrue(), cmp_limit, bol_limit);
> 1696: 

New line can be removed
Suggestion:

src/hotspot/share/opto/loopnode.cpp line 1713:

> 1711:   Node* limit = nullptr;
> 1712:   Node* cmp = loop_exit_test(back_control, loop, incr, limit, bt, cl_prob);
> 1713: 

Not sure if that new line was intended
Suggestion:

src/hotspot/share/opto/loopnode.hpp line 1229:

> 1227:   bool is_counted_loop_with_speculative_long_limit(Node* x, IdealLoopTree*& loop, BasicType iv_bt);
> 1228:  private:
> 1229:   bool do_is_counted_loop(Node* x, IdealLoopTree*& loop, BasicType iv_bt);

Was like that before but I think `is_counted_loop()` is a bit misleading, suggesting it's a query but it's actually doing the conversion work. Since you now change these methods anyway, what do you think about the following naming suggestions?
- public `try_convert_to_counted_loop()`
  - calls: private `convert_to_counted_loop()`
  - call: private `convert_to_counted_loop_with_speculative_long_limit()`

test/hotspot/jtreg/compiler/loopopts/TestIntCountedLoopLongLimit.java line 41:

> 39:  */
> 40: public class TestIntCountedLoopLongLimit {
> 41:     private static final Random RNG = jdk.test.lib.Utils.getRandomInstance();

I suggest to rather use an `import` instead of a fully qualified name:
Suggestion:

import jdk.test.lib.Asserts;
import jdk.test.lib.Utils;

import java.util.Random;

/**
 * @test
 * @bug 8336759
 * @summary test long limits in int counted loops are speculatively converted to int for counted loop
 *         optimizations
 * @library /test/lib /
 * @requires vm.compiler2.enabled
 * @run driver compiler.loopopts.TestIntCountedLoopLongLimit
 */
public class TestIntCountedLoopLongLimit {
    private static final Random RNG = Utils.getRandomInstance();

test/hotspot/jtreg/compiler/loopopts/TestIntCountedLoopLongLimit.java line 88:

> 86:             "testCountedLoopWithSwappedComparisonOperand" })
> 87:     public static void runTestSimpleCountedLoops(RunInfo info) {
> 88:         long limit = RNG.nextLong(0, 1024 * 1024); // Choice a small number to avoid tests taking too long

Suggestion:

        long limit = RNG.nextLong(0, 1024 * 1024); // Choose a small number to avoid tests taking too long

test/hotspot/jtreg/compiler/loopopts/TestIntCountedLoopLongLimit.java line 127:

> 125: 
> 126:     // Use a larger stride to avoid tests taking too long
> 127:     private static final int LARGE_STRIDE = Integer.MAX_VALUE / 1024;

Can you move this to the top of the class to the other static final field?

test/hotspot/jtreg/compiler/loopopts/TestIntCountedLoopLongLimit.java line 179:

> 177:     }
> 178: 
> 179:     private static volatile long SOME_LONG = 42;

Same here, can you move this to the top of the class?

-------------

PR Review: https://git.openjdk.org/jdk/pull/22449#pullrequestreview-2474951014
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867278568
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867318157
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867324663
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867330528
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867342583
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867306191
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867305838
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867305458
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867304899
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867349691
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867351852
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867353100
PR Review Comment: https://git.openjdk.org/jdk/pull/22449#discussion_r1867353558