RFR(M): 8080289: Intermediate writes in a loop not eliminated by optimizer

[Vladimir Kozlov]

We don't call previous node as 'user' - we call them definitions or 
inputs. Your comment change in memnode.cpp sound strange because of 
that. The original statement was correct:
// If anybody other than 'this' uses 'mem', we cannot fold 'mem' away.

in your case it will be:

// If anybody other than 'this' uses 'st', we cannot fold 'st' away.

Also code does not seems right. The code should go through input memory 
chain and remove all preceding similar stores - 'this' node remains and 
we change its memory input which makes previous stores dead. So you 
can't do 'prev = st'.

You need to set improved = true since 'this' will not change. We also 
use 'make_progress' variable's name in such cases.

I try_move_store_before_loop() add check (n_loop->_head == n_ctrl) to 
make sure it is not other control node inside loop. Then you can check 
Phi's control as (mem->in(0) == n_ctrl).

I don't understand verification code "Check that store's control does 
post dominate loop entry". In first comment you said "Store has to be 
first in the loop body" - I understand this as Store's control should be 
loop's head. You can't allow store to be on one of branches (it will not 
post dominate) but your check code allow that. Also the check done only 
in debug VM.

If you really want to accept cases when a store is placed after diamond 
control then you need checks in product to make sure that it is really 
post dominate head. For that I think you need to go from loopend to 
loophead through idom links and see if you meet n_ctrl.

I don't see assert(n->in(0) in try_move_store_before_loop() but you have 
it in try_move_store_after_loop().

Why you need next assert?:
+         assert(n_loop != address_loop, "address is loop varying");

Should you check phi == NULL instead of assert to make sure you have 
only one Phi node?

conflict between assert and following check:
+               assert(new_loop->_child != NULL, "");
+               if (new_loop->_child == NULL) new_loop->_body.push(st);

Thanks,
Vladimir

On 6/10/15 8:03 AM, Roland Westrelin wrote:
> http://cr.openjdk.java.net/~roland/8080289/webrev.00/
>
> Sink stores out of loops when possible:
>
> for (int i = 0; i < 1000; i++) {
>      // Some stuff that doesn’t prevent the optimization
>      array[idx] = i;
> }
>
> becomes:
>
> for (int i = 0; i < 1000; i++) {
>      // Some stuff
> }
> array[idx] = 999;
>
> Or move stores before the loop when possible:
>
> for (int i = 0; i < 1000; i++) {
>      array[idx] = 999;
>      // Some stuff that doesn’t prevent the optimization
> }
>
> becomes:
>
> array[idx] = 999;
> for (int i = 0; i < 1000; i++) {
>      // Some stuff
> }
>
> The change in memnode.cpp is useful to clean up code generated from test_after_5 because the stores are moved out of the loop only after the loop is split and unrolled. That code removes duplicate stores.
>
> Roland.
>