RFR: 8304948: [vectorapi] C2 crashes when expanding VectorBox [v2]

[Tobias Hartmann]

On Tue, 18 Apr 2023 14:53:54 GMT, Eric Liu <eliu at openjdk.org> wrote:

>> src/hotspot/share/opto/vector.cpp line 307:
>> 
>>> 305: void PhaseVector::expand_vbox_node(VectorBoxNode* vec_box) {
>>> 306:   if (vec_box->outcnt() > 0) {
>>> 307:     VectorSet visited;
>> 
>> Do we need a ResourceMark here or above?
>
> expand_vbox_node only has a single one call chain, which start from Compile::Optimize(). I think we can trust the ResourceMark defined in there [1].
> 
> [1] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/compile.cpp#L2207

Makes sense.

>> src/hotspot/share/opto/vector.cpp line 327:
>> 
>>> 325:   // Phi (VectorBox VectorBox) => VectorBox (Phi Phi)
>>> 326:   if (visited.test_set(vbox->_idx)) {
>>> 327:     assert(vbox->is_Phi(), "not a phi");
>> 
>> Are we sure that the cycle is always detected at a phi?
>
> I think it is.
> 
> The CYCLE is derived from merge_through_phi, in which only Phi CYCLE is allowed [1].
> 
> This method `expand_vbox_node_helper` serves to locate the target node `Proj(VectorBoxAllocate)` and then replace it with a punch of other nodes. When expanding VectorBox, the normal case is `VectorBox (Proj value)`. If it is in shape of `VectorBox (Phi1 Phi2)`[2] or `VectorBox (Phi1 vect)`[3],  I suppose it should be transformed by merge_through_phi.
> 
> In `expand_vbox_node_helper`, it is recursive only at Phi1 since it is where Proj in. 
> 
> [1] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/cfgnode.cpp#L2574
> [2] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/vector.cpp#L331
> [3] https://github.com/e1iu/jdk/blob/ENTLLT-4778-external/src/hotspot/share/opto/vector.cpp#L341

Okay, thanks for the details! Looks good.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13489#discussion_r1170982063
PR Review Comment: https://git.openjdk.org/jdk/pull/13489#discussion_r1170982799