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

Vitaly Davidovich vitalyd at gmail.com
Wed Jun 17 19:08:19 UTC 2015

If we can guarantee that all passed stores are normal (I assume we will have barriers otherwise in between) then I agree. I am not sure why we didn't do it before, there could be a counterargument for that which I don't remember. To make sure, ask John.

Just my $.02 in here, but AFAIK, this should be completely legal assuming no barriers in there, as you say. Java doesn't allow introducing new writes, but removing existing ones like this should be legal and a good optimization.

On Wed, Jun 17, 2015 at 3:03 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com

wrote:

> http://gee.cs.oswego.edu/dl/jmm/cookbook.html > > it’s allowed to reorder normal stores with normal stores

If we can guarantee that all passed stores are normal (I assume we will have barriers otherwise in between) then I agree. I am not sure why we didn't do it before, there could be a counterargument for that which I don't remember. To make sure, ask John. >> We need to call Ideal() again if store inputs are changed. So if st2 is removed then inputs of st1 are changed so we need to rerun Ideal(). This allow to avoid having your new loop in the Ideal(). > > Sorry, I don’t understand this. Are you saying there’s no need for a loop at all? Or are you saying that as soon as there’s a similar store that is found we should return from Ideal that will be called again to maybe find other similar stores? Yes, it may simplify the code of Ideal. You may still need a loop to look for previous store which could be eliminated but you don't need to have 'prev'. As soon you remove one node, you exit Ideal returning 'this' and it will be called again so you can search for another previous store. >> BOTTOM (all slices) Phi? > > Wouldn’t there be a MergeMem between the store and the Phi then? Yes. Okay, you can keep the check as assert we will see if Nightly testing hit it it or not. Thanks, Vladimir

On 6/17/15 1:35 AM, Roland Westrelin wrote:

That’s what I think the code does. That is if you have:

st1->st2->st3->st4

I assume st4 is first store and st1 is last. Right? Program order is: st4 st3 st2 st1 and st3 is redundant with st1, the chain should become: st1->st2->st4 I am not sure it is correct optimization. On some machines result of st3 could be visible before result of st2. And you change it. I am suggesting not do that. Do you need that for stores move from loop? It’s not required. It cleans up the graph in some cases like this: static void testafter5(int idx) { for (int i = 0; i < 1000; i++) {_ _array[idx] = i;_ _array[idx+1] = i;_ _array[idx+2] = i;_ _array[idx+3] = i;_ _array[idx+4] = i;_ _array[idx+5] = i;_ _}_ _}_ _all stores are sunk out of the loop but that happens after iteration_ _splitting and so there are multiple redundant copies of each store that are_ _not collapsed._ _This said, we currently reorder the stores even if it’s less aggressive_ _than what I’m proposing. With program:_ _st4_ _st3_ _st2_ _st1_ _If st1, st3 and st4 are on one slice and st2 is on another and if st1 and_ _st3 store to the same address we optimize st3 out:_ _st4_ _st2_ _st1_ _so st3=st1 may only be visible after st2._ _Also, the way I read the first table in this:_ _http://gee.cs.oswego.edu/dl/jmm/cookbook.html_ _it’s allowed to reorder normal stores with normal stores_ _so we need to change the memory input of st2 when we find st3 can be_ _removed. In the code, at that point, this=st1, st = st3 and prev=st2._ _In this case the code should be:_ _if (st->in(MemNode::Address)->eqvuncast(address) && ... } else { prev = st; } to update 'prev' with 'st' only if 'st' is not removed. You’re right. Also, I think, st->in(MemNode::Memory) could be put in local var since it is used several times in this code. You need to set improved = true since 'this' will not change. We also use 'makeprogress' variable's name in such cases.

In the example above, if we remove st2, we modify this, right? We need to call Ideal() again if store inputs are changed. So if st2 is removed then inputs of st1 are changed so we need to rerun Ideal(). This allow to avoid having your new loop in the Ideal(). Sorry, I don’t understand this. Are you saying there’s no need for a loop at all? Or are you saying that as soon as there’s a similar store that is found we should return from Ideal that will be called again to maybe find other similar stores? We’ll find a path from the head that doesn’t go through the store and that exits the loop. What the comment doesn’t say is that with example 2 below: for (int i = 0; i < 10; i++) {_ _if (somecondition) {_ _uncommontrap();_ _}_ _array[idx] = 999;_ _}_ _my verification code would find the early exit as well._ _It’s verification code only because if we have example 1 above, then we_ _have a memory Phi to merge both branches of the if. So the pattern that we_ _look for in PhaseIdealLoop::trymovestorebeforeloop() won’t match: the_ _loop’s memory Phi backedge won’t be the store. If we have example 2 above,_ _then the loop’s memory Phi doesn’t have a single memory use. So I don’t_ _think we need to check that the store post dominate the loop head in_ _product. That’s my reasoning anyway and the verification code is there to_ _verify it._ _I missed 'mem->in(LoopNode::LoopBackControl) == n' condition. Which reduce cases only to one store to this address in the loop - good. How you check in product VM that there are no other exists from a loop (your example 2)? Is it guarded by mem->outcnt() == 1 check? Yes. Should you check phi == NULL instead of assert to make sure you have only one Phi node? Can there be more than one memory Phi for a particular slice that has in(0) == nloop->head? I would have expected that to be impossible. BOTTOM (all slices) Phi? Wouldn’t there be a MergeMem between the store and the Phi then? For the record, the webrev: http://cr.openjdk.java.net/~roland/8080289/webrev.00/ Roland. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150617/884230e1/attachment.html>

More information about the hotspot-compiler-dev mailing list