review for 7032963: StoreCM shouldn't participate in store elimination

[Tom Rodriguez]

On Apr 1, 2011, at 4:37 PM, Vladimir Kozlov wrote:

> An other problem if n is on a branch and you could eliminate dominated StoreCM which above the split point resulting in not having StoreCM on opposite branch.

You mean:

a.f = x
b.f = y;
if (test)
  return
a.b = c;

The StoreCM for a.f has a single user but it's used by the StoreCM of b.f which has multiple users.  So I think the search needs to stop when it encounters multiple users of a StoreCM since that represents a split of control flow.  Thanks for catching that.

Sounds like a job for partial redundancy elimination.

tom

> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Apr 1, 2011, at 3:47 PM, Vladimir Kozlov wrote:
>>> You may put n->in(MemNode::Address) and n->in(MemNode::ValueIn) into locals before the loop. Also you need to kill the node explicitly otherwise it still be connected to its inputs:
>>> 
>>> +         // Eliminate the previous StoreCM
>>> +         prev->set_req(MemNode::Memory, mem->in(MemNode::Memory));
>>> +         assert(mem->outcnt() == 0, "should be dead");
>>> +         mem->disconnect_inputs(NULL);
>> I'll have to rework the mem traversal a little.  Actually I think there might have been a bug with the old code since it always updated prev.  I believe this is correct:
>>        // Eliminate the previous StoreCM                                                                                                                     prev->set_req(MemNode::Memory, mem->in(MemNode::Memory));
>>        assert(mem->outcnt() == 0, "should be dead");
>>        mem->disconnect_inputs(NULL);
>>      } else {                                                                                                                                               prev = mem;                                                                                                                                         }
>>      mem = prev->in(MemNode::Memory);
>>    }
>> I think I'll put together a little test case to make sure this is working correctly.
>> tom
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> I could push this to hotspot-gc so it gets more CMS testing .
>>>> http://cr.openjdk.java.net/~never/7032963
>>>> 7032963: StoreCM shouldn't participate in store elimination
>>>> Reviewed-by:
>>>> StoreCM shouldn't participate in redundant store elimination since
>>>> that could violate the requirement that a StoreCM must be strictly
>>>> after a field update.  This results in a large number of redundant
>>>> StoreCMs being emitted for blocks of fields updates, so I added an
>>>> optimization to fold them up safely.  Previously the extra dependence
>>>> was converted into a precedence edge just before register allocation
>>>> but I moved this logic into final_graph_reshape.  I then added logic
>>>> to search through chains of StoreCMs to eliminate earlier redundant
>>>> ones and transfer their precedence edges to the one that is kept.
>>>> This ensures that they are scheduled properly.  This actually
>>>> eliminates duplicates that were previously missed so the code quality
>>>> is slightly better.  Tested by inspecting code generation with script
>>>> to identify duplicates.  Also ran CTW with -XX:+UseCondCardMark and
>>>> -XX:+UseG1GC.