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

[Vladimir Kozlov]

And, it seems, your current code covers this case already. So my false 
assumption helped you to find the real problem ;)

Thanks,
Vladimir

Vladimir Kozlov wrote:
> 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.
>>