RFR: 8263164: assert(_base >= VectorA && _base <= VectorZ) failed: Not a Vector while calling StoreVectorNode::memory_size()

[Vladimir Kozlov]

On Tue, 9 Mar 2021 23:28:00 GMT, Jie Fu <jiefu at openjdk.org> wrote:

>> I don't think it is correct place to fix it. Why checks in StoreNode(and LoadNode)::Value() does not work for vectors?:
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L2702
>> 
>> Did we forgot to put this nodes on IGVN worklist somewhere?
>
>> I don't think it is correct place to fix it. Why checks in StoreNode(and LoadNode)::Value() does not work for vectors?:
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L2702
>> 
>> Did we forgot to put this nodes on IGVN worklist somewhere?
> 
> Hi @vnkozlov ,
> 
> Thanks for your review.
> 
> After more investigation, I found the same issue for non-vector store nodes.
> For example, StoreN was also observed with in(MemNode::ValueIn)->bottom_type() == Type::TOP during PhaseIterGVN::optimize().
> 
> The nodes should have been pushed on the IGVN worklist since it happened when StoreN was popped from the IGVN worklist.
> So I think StoreVectorNode::memory_size() should also work when in(MemNode::ValueIn)->bottom_type() == Type::TOP during PhaseIterGVN::optimize().
> 
> By the way, I'm not clear why you say the checks in StoreNode(and LoadNode)::Value() does not work for vectors?
> Could you make it more clearer?
> Thanks.
> 
> Best regards,
> Jie

What I mean is IGVN should replace nodes which have TOP input with TOP and remove them from graph as dead nodes.
`Value()` methods do replacement after checking node's inputs for TOP. And `Ideal()` method should check for TOP and immediately return to avoid unneeded transformations (or guard transformation code with `!= TOP` check) and let `Value()` do replacement with TOP.

In case of memory nodes `MemNode::Ideal_common()` do such checks (but only for ctrl, mem and adr inputs):
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L286

To my surprise `StoreNode::Ideal()` does not check `in(MemNode::ValueIn)` input for TOP at all. May be because in normal case we don't do much for `in(MemNode::ValueIn)` when processing Store nodes and eventually `StoreNode::Value()` will replace node with TOP.

Yes, your fix works for `Vector` nodes. But as you saw normal `Store` nodes also can have TOP value input and we do useless transformations until StoreNode::Value() calls. I think the better fix is to check ` in(MemNode::ValueIn)`  for TOP in `StoreNode::Ideal()`.

And we need to test it hard because it is very old code.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2867