RFR: 8308994: C2: Re-implement experimental post loop vectorization [v2]

[Pengfei Li]

On Fri, 23 Jun 2023 12:24:57 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Pengfei Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address part of comments from Emanuel
>
> src/hotspot/share/opto/vmaskloop.cpp line 71:
> 
>> 69:   if (cl->loopexit()->in(0) != cl) return;
>> 70:   // Skip if some loop operations are pinned to the backedge
>> 71:   if (cl->back_control()->outcnt() != 1) return;
> 
> It would be interesting to have some trace flag that tells us why we bailed out here and did not do the post-loop vectorization. Unless of course it becomes too noisy.

Great suggestion! Done.

> src/hotspot/share/opto/vmaskloop.cpp line 104:
> 
>> 102:   _core_set.clear();
>> 103:   _body_set.clear();
>> 104:   _body_nodes.clear();
> 
> Would it make sense to somehow reserve the space, so that we do not allocate multiple times when growing these data structures later?

Could you elaborate how to do such reservation in C2? Just allocation with some larger sizes at the beginning? Or any other examples to refer?

> src/hotspot/share/opto/vmaskloop.cpp line 172:
> 
>> 170:   if (idx != -1) {
>> 171:     trace_msg(nullptr, "Loop has unreachable node while traversing from head");
>> 172:     return false;
> 
> Can this ever happen? Or could you add an assert here?

Yes, it happened before. I will try to find a case.

> src/hotspot/share/opto/vmaskloop.cpp line 214:
> 
>> 212:     }
>> 213:   } else if (in->is_Phi()) {
>> 214:     // 2) We don't support phi nodes except the iv phi of the loop
> 
> Add: and memory phi's cannot be reached.

Done

> src/hotspot/share/opto/vmaskloop.cpp line 223:
> 
>> 221:       return true;
>> 222:     } else {
>> 223:       trace_msg(in, "Found unsupported memory load input");
> 
> This is a bit generic. Would be nice to have more specific info why it is "unsupported". See my example that hit it.

Good suggestion! I have added more `trace_msg()` calls in `VectorMaskedLoop::mem_access_to_swpointer`.

> src/hotspot/share/opto/vmaskloop.cpp line 269:
> 
>> 267:     Node_List* worklist = new Node_List(_arena);
>> 268:     if (!collect_statements_helper(store, MemNode::ValueIn, stmt, worklist)) {
>> 269:       return false;
> 
> Why does the `store` need special handling here? Can you not just throw it on the `worklist`? Would be nice to have the code be shorter ;)

Good catch! Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250481354
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250488640
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250483771
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250492279
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250494389
PR Review Comment: https://git.openjdk.org/jdk/pull/14581#discussion_r1250492173