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

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Jun 10 19:31:20 UTC 2015

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?:

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

conflict between assert and following check:

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 testafter5 because the stores are moved out of the loop only after the loop is split and unrolled. That code removes duplicate stores. Roland.

More information about the hotspot-compiler-dev mailing list