RFR: 8350485: C2: factor out common code in Node::grow() and Node::out_grow() [v2]

[Roberto Castañeda Lozano]

On Mon, 10 Mar 2025 14:06:36 GMT, Saranya Natarajan <duke at openjdk.org> wrote:

>> Node:grow() and Node::out_grow() are copy-pasted from each other and their core logic could be factored out into a third function or at least cleaned up. Hence,the fix includes a function Node::array_resize() that implements the core logic of Node::grow() and Node::out_grow(). 
>> 
>>  Link to Github action which had no failures :  https://github.com/sarannat/jdk/actions/runs/13677508359
>
> Saranya Natarajan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8350485: Addressing review comments on code style

Thanks for cleaning up this code, Saranya! I have a few more style/naming suggestions, the refactored logic looks otherwise good to me.

src/hotspot/share/opto/node.cpp line 667:

> 665: }
> 666: 
> 667: // Resize input or output array to grow it the next larger power-of-2 bigger

Suggestion:

// Resize input or output array to grow it to the next larger power-of-2 bigger

src/hotspot/share/opto/node.cpp line 669:

> 667: // Resize input or output array to grow it the next larger power-of-2 bigger
> 668: // than len.
> 669: void Node::resize_array(Node**& array, node_idx_t& max_size, uint len, bool is_input_array) {

This is subjective, but I would find it clearer if the name of `bool is_input_array` reflected what does `resize_array` needs to do with `array` rather than what is the source/origin of `array`. My suggestion would be something like `bool needs_clearing`, `bool initialize_to_null`, or similar.

src/hotspot/share/opto/node.cpp line 686:

> 684:   // Trimming to limit allows a uint8 to handle up to 255 edges.
> 685:   // Previously I was using only powers-of-2 which peaked at 128 edges.
> 686:   //if( new_max >= limit ) new_max = limit-1;

I suggest to remove this line that was already commented out before this changeset.

src/hotspot/share/opto/node.cpp line 689:

> 687:   if (!is_input_array) {
> 688:     assert(array != nullptr && array != NO_OUT_ARRAY, "out must have sensible value");
> 689:   }

This is somewhat subjective, but I prefer to inline the `!is_input_array` pre-condition into the assertion itself, for compactness.
Suggestion:

  assert(is_input_array || (array != nullptr && array != NO_OUT_ARRAY), "out must have sensible value");

src/hotspot/share/opto/node.cpp line 697:

> 695:   // This assertion makes sure that Node::_max is wide enough to
> 696:   // represent the numerical value of new_max.
> 697:   assert(max_size == new_max && max_size > len, "int width of _max is too small");

This is pre-existing, but I think it is worth simplifying anyway:
Suggestion:

  assert(max_size > len, "int width of _max is too small");

src/hotspot/share/opto/node.cpp line 708:

> 706: //-----------------------------out_grow----------------------------------------
> 707: // Grow the input array, making space for more edges
> 708: void Node::out_grow( uint len ) {

For style consistency with `Node::grow`:

Suggestion:

void Node::out_grow(uint len) {

src/hotspot/share/opto/node.hpp line 339:

> 337:   void out_grow( uint len );
> 338:   // Resize input or output array to grow it the next larger power-of-2 bigger
> 339:   // than len.

Suggestion:

  // Resize input or output array to grow it to the next larger power-of-2
  // bigger than len.

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

PR Review: https://git.openjdk.org/jdk/pull/23928#pullrequestreview-2673440209
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988721217
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988764754
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988740998
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988745651
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988738802
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988754817
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1988715587