RFR: 8278784: C2: Refactor PhaseIdealLoop::remix_address_expressions() so it operates on longs [v3]

[Tobias Hartmann]

On Wed, 5 Jan 2022 16:07:58 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The logic in PhaseIdealLoop::remix_address_expressions() that's
>> specific to int nodes apply equally to long nodes. This change
>> refactors that code so it applies to both integer types. This improves
>> performance on some of panama's micro-benchmarks.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Christian's review

Overall looks good to me but there a lots of places with inconsistent pointer asterisk placement (I started marking some). I don't have a strong opinion though, I just like consistency :)

src/hotspot/share/opto/loopopts.cpp line 359:

> 357: Node* PhaseIdealLoop::remix_address_expressions_add_left_shift(Node* n, IdealLoopTree* n_loop, Node* n_ctrl, BasicType bt) {
> 358:   assert(bt == T_INT || bt == T_LONG, "only for integers");
> 359:   int n_op = n->Opcode();

Seems to be unused or only used once with my suggested change. Maybe remove.

src/hotspot/share/opto/loopopts.cpp line 361:

> 359:   int n_op = n->Opcode();
> 360: 
> 361:   if (n->Opcode() == Op_LShift(bt)) {

Suggestion:

  if (n_op == Op_LShift(bt)) {

src/hotspot/share/opto/loopopts.cpp line 365:

> 363:     Node* scale = n->in(2);
> 364:     Node* scale_ctrl = get_ctrl(scale);
> 365:     IdealLoopTree *scale_loop = get_loop(scale_ctrl);

Suggestion:

    IdealLoopTree* scale_loop = get_loop(scale_ctrl);

src/hotspot/share/opto/loopopts.cpp line 376:

> 374:     Node* add = n->in(1);
> 375:     Node* add_ctrl = get_ctrl(add);
> 376:     IdealLoopTree *add_loop = get_loop(add_ctrl);

Suggestion:

    IdealLoopTree* add_loop = get_loop(add_ctrl);

src/hotspot/share/opto/loopopts.cpp line 387:

> 385:       Node* zero = _igvn.integercon(0, bt);
> 386:       set_ctrl(zero, C->root());
> 387:       Node *neg = SubNode::make(zero, add->in(2), bt);

Suggestion:

      Node* neg = SubNode::make(zero, add->in(2), bt);

src/hotspot/share/opto/loopopts.cpp line 395:

> 393:     assert(add->Opcode() == Op_AddI || add->Opcode() == Op_AddL, "");
> 394:     // See if one add input is loop invariant
> 395:     Node *add_var = add->in(1);

Suggestion:

    Node* add_var = add->in(1);


Same below.

src/hotspot/share/opto/loopopts.cpp line 434:

> 432: // moved out.  We'd like to do all associative operators, but it's especially
> 433: // important (common) to do address expressions.
> 434: Node *PhaseIdealLoop::remix_address_expressions(Node *n) {

Suggestion:

Node* PhaseIdealLoop::remix_address_expressions(Node* n) {


Many more below.

test/hotspot/jtreg/compiler/c2/irTests/TestRemixAddressExpressions.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Red Hat, Inc. All rights reserved.

Suggestion:

 * Copyright (c) 2022, Red Hat, Inc. All rights reserved.

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6892