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

[Christian Hagedorn]

On Mon, 20 Dec 2021 10:01:29 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.

Otherwise, the refactoring looks good! The comments are mostly about code style and further refactoring suggestions of touched existing code. Nice IR tests!

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

> 1443:   // moved out.  We'd like to do all associative operators, but it's especially
> 1444:   // important (common) to do address expressions.
> 1445:   Node* remix_address_expressions(Node *n);

You could also update the asterisk at the parameter to `Node* n`.

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

> 354: }
> 355: 
> 356: Node* PhaseIdealLoop::remix_address_expressions_helper(Node* n, IdealLoopTree* n_loop, Node* n_ctrl, BasicType bt) {

The method just seem to handle `// Replace expressions like ((V+I) << 2) with (V<<2 + I<<2).`. Could the method name be updated to `remix_address_expressions_left_shift` or something similar to better reflect this? You could also move this comment up to get it as method comment instead.

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

> 359:   // Replace expressions like ((V+I) << 2) with (V<<2 + I<<2).
> 360:   if (n->Opcode() == Op_LShift(bt)) {
> 361:     assert(n_op == Op_LShiftI || n_op == Op_LShiftL, "");

Is the assertion (and the one on L392) really necessary? `Op_xx()` always either returns `Op_xxI` or `Op_xxL`. Maybe we could just assert that `bt` is either `T_INT` or `T_LONG` on method entry instead as sanity check.

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

> 375:     Node* add_ctrl = get_ctrl(add);
> 376:     IdealLoopTree *add_loop = get_loop(add_ctrl);
> 377:     //assert( n_loop == add_loop, "" );

Probably a left-over from the bailout below. Could be removed.

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

> 376:     IdealLoopTree *add_loop = get_loop(add_ctrl);
> 377:     //assert( n_loop == add_loop, "" );
> 378:     if (n_loop != add_loop) return NULL;  // happens w/ evil ZKM loops

You could also add braces here as above.

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

> 399:     IdealLoopTree *add_invar_loop = get_loop(add_invar_ctrl);
> 400:     if (add_var_loop == n_loop) {
> 401:     } else if (add_invar_loop == n_loop) {

How about changing that to


if (add_invar_loop == n_loop) {
  // Swap to find the invariant part
  ...
} else if (add_var_loop != n_loop) {
  return NULL:
}

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

> 406:       add_var = add->in(2);
> 407:       Node *add_var_ctrl = get_ctrl(add_var);
> 408:       IdealLoopTree *add_var_loop = get_loop(add_var_ctrl);

The variable is unused. Is `get_loop()` just meant as sanity check? I first thought it accidentally shadows the previous variable definition on L396 instead of updating the variable but it is unused afterwards.

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

> 466:   }
> 467: 
> 468:   int n_op = n->Opcode();

Could be moved down to L480.

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

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