RFR: 8350485: C2: factor out common code in Node::grow() and Node::out_grow()
Emanuel Peter
epeter at openjdk.org
Mon Mar 10 10:58:27 UTC 2025
On Thu, 6 Mar 2025 09:08:41 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
@sarannat I'm mostly leaving code style comments. I promise I won't be so pedantic in the future ;)
It may be good for you to read through this, at least to have an overview:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
src/hotspot/share/opto/node.cpp line 667:
> 665: }
> 666:
> 667: //------------------------------resize_array-------------------------------------------
Suggestion:
Just FYI. You can have these headers, but it is not expected any more. So I usually leave them out. But up to you.
src/hotspot/share/opto/node.cpp line 670:
> 668: // Resize input or output array to grow it the next larger power-of-2 bigger
> 669: // than len.
> 670: void Node::resize_array( Node**& array, node_idx_t& max_size, uint len, bool is_in) {
Suggestion:
void Node::resize_array(Node**& array, node_idx_t& max_size, uint len, bool is_in) {
Code style
src/hotspot/share/opto/node.cpp line 673:
> 671: Arena* arena = Compile::current()->node_arena();
> 672: uint new_max = max_size;
> 673: if( new_max == 0 ) {
Suggestion:
if(new_max == 0) {
While we are here we might as well fix this too.
src/hotspot/share/opto/node.cpp line 675:
> 673: if( new_max == 0 ) {
> 674: max_size = 4;
> 675: array = (Node**)arena->Amalloc(4*sizeof(Node*));
Suggestion:
array = (Node**)arena->Amalloc(4 * sizeof(Node*));
We generally want to have spaces around operators.
See:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
> Use spaces around operators, especially comparisons and assignments. (Relaxable for boolean expressions and high-precedence operators in classic math-style formulas.)
src/hotspot/share/opto/node.cpp line 683:
> 681: }
> 682: return;
> 683: }
Suggestion:
}
return;
}
Indentation
src/hotspot/share/opto/node.hpp line 340:
> 338: // Resize input or output array to grow it the next larger power-of-2 bigger
> 339: // than len.
> 340: void resize_array(Node **&array, node_idx_t &max_size, uint len, bool is_in);
Suggestion:
void resize_array(Node**& array, node_idx_t &max_size, uint len, bool is_in);
Nit code style ;)
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/23928#pullrequestreview-2667282496
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1985077851
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1985076647
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1985078681
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1985080375
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1985080970
PR Review Comment: https://git.openjdk.org/jdk/pull/23928#discussion_r1985075533
More information about the hotspot-compiler-dev
mailing list