RFR: 8310190: C2 SuperWord: AlignVector is broken, generates misaligned packs [v6]

[Emanuel Peter]

On Sat, 7 Oct 2023 00:47:26 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed up verify_vector_alignment in ad file
>
> src/hotspot/share/opto/compile.cpp line 3684:
> 
>> 3682:         jlong alignment = MIN2(memory_size, (jlong)ObjectAlignmentInBytes);
>> 3683:         assert(2 <= alignment && alignment <= 64, "alignment must be in range");
>> 3684:         assert(is_power_of_2(alignment), "alignment must be power of 2");
> 
> Do we have somewhere check that vector size is always power of 2 or multiply of object alignment (for case when it is bigger than object alignment)?

Yes, at least we do those checks in SuperWord. We only ever create power of 2 vector sizes. And `ObjectAlignmentInBytes` is verified to also always be power of 2.
These asserts here are just there to catch any issues if this assumption is ever broken in the future.

> src/hotspot/share/opto/superword.cpp line 2924:
> 
>> 2922: #ifdef ASSERT
>> 2923:       // Mark Load/Store Vector for alignment verification
>> 2924:       if (AlignVector && VerifyAlignVector && cl->is_main_loop()) {
> 
> `AlignVector &&` no need if you do check when VerifyAlignVector is set.

Done

> src/hotspot/share/opto/superword.cpp line 2927:
> 
>> 2925:         if (vn->Opcode() == Op_LoadVector) {
>> 2926:           vn->as_LoadVector()->set_must_verify_alignment();
>> 2927:         } else if (vn->Opcode() == Op_StoreVector) {
> 
> Do you mark all Load/StoreVector nodes for verification? Then why do you even need to set such field?
> I you verify only few you need comment explaining which one you are verifying.

I will add a comment. We only mark the nodes created by SuperWord, because AlignVector does only apply to SuperWord, and not for example to VectorAPI nodes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14785#discussion_r1356556538
PR Review Comment: https://git.openjdk.org/jdk/pull/14785#discussion_r1356557051
PR Review Comment: https://git.openjdk.org/jdk/pull/14785#discussion_r1356558736