RFR(M): 8007294: ReduceFieldZeroing doesn't check for dependent load and can lead to incorrect execution

[Roland Westrelin]

Thanks for taking a look at this, Vladimir.

> In memnode.cpp could you explain how you can get old_mem->outcnt() == 0 in new check when it has user (this node):  Node *mem = in(MemNode::Memory) ?

322     set_req(MemNode::Memory, mem);

can disconnect old_mem from the current node.

> In new code in can_capture_store():
> 
>  Move "(n == st)" check before control edge check which is more expensive.

Ok.

>  Don't check (n->as_MergeMem()->memory_at(alias_idx) == m) because m could be IdxBot slice and later load could still reference alias_idx slice through it. Just push uses of all MergeMem nodes.

If all MergeMem nodes are pushed I think we could end up looking at memory operations that are after the store and be too conservative.
I don't understand why it's not sufficient to test n->as_MergeMem()->memory_at(alias_idx) == m. Either n->in(alias_idx) is not top and and it's not m that means there's something modifying the memory state on this slice, so something that must be the Store or a subsequent store and the uses of n happen after the Store so can be ignored. Or n->in(alias_idx) is m then we look at uses of n. Or n->in(alias_idx) is top and n->as_MergeMem()->memory_at(alias_idx) returns n->in(Compile::AliasIdxBot) and if it's m (there's nothing between the Initialize node and n) then we look at uses of n.

>  I think we should be more conservative with StrIntrinsic, you can't relay on address type because these intrinsics have 2 inputs and it may read before the store since one of them has path to store's memory.
>  Your alias_idx compare for memory node will not works for (EA) instances types since their alias idx will be different from general alias idx.

Wouldn't the new alias for an EA instance be propagated to all its uses so that it doesn't matter? We're looking for a load and store on the same instance. Wouldn't they always have the same alias idx?

>  You are also making decision (failed = false) in parser phase when load is not generated yet so you may miss the case.

If a Load happens after a Store in parse order then it's the expected behavior that the Load sees the modified memory and so there's no need to worry. At parse time, all Loads that are between the Initialize and the Store are already parsed.

> In phaseX.cpp new check "} else if (ReduceFieldZeroing" could be skipped by next preceding code if there is only one additional user (Store):
>             } else if (in->outcnt() == 1 &&
>                        in->has_special_unique_user()) {
>               _worklist.push(in->unique_out());

Ok.

> Use explicit check (in->in(0) != NULL)

Ok.

> Also I think you need to add check (i == MemNode::Memory) to do that only for loads which depends on this Initialize node and memory projection.

Ok.

Roland.