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

[Vladimir Kozlov]

Actually I thought about slightly different case:

a.f = x
if (test) {
   a.b = y;
}

But StoreCM for a.f should have several users (StoreCM for a.b and mergemem) so 
your condition (stop serch if multiple users) stays true.

Vladimir

Tom Rodriguez wrote:
> 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.
>