RFR: 8300258: C2: vectorization fails on simple ByteBuffer loop [v2]

[Vladimir Kozlov]

On Fri, 24 Feb 2023 08:08:17 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I am not sure that it is correct to relax the check to only bailout for misaligned access to the same array. Unless I forgot how alias indexes created. Do we have the same alias index or different for 2 arrays in `testByteByte1()` test?
>
>> Do we have the same alias index or different for 2 arrays in testByteByte1() test?
> 
> @vnkozlov as explained in https://github.com/openjdk/jdk/pull/12440#discussion_r1116622180, SuperWord seems to have all arrays of the same basic type in the same slice. If a method has two input arrays, they must be in the same slice, because we do not know if it is maybe the same array.
> 
> However, if the two arrays have different basic types, then they are in separate memory slices.
> 
> Basically, @rwestrel has detected a subtle difference between `memory slice` and `velt_type`. Usually, the two are identical. But with `Unsafe`, the type of the load/store can be different to that of the array.
> 
> @rwestrel : can you change the name of the Bug to reflect this? Suggestion:
> `C2: SuperWord alignment analysis must be based on memory slice, not velt type`
> 
> Question: is it possible to somehow use `Unsafe` to extract the memory address of an array? And then maybe store that into another array of a different type? I hope `Unsafe` has been designed in such a way that we cannot ever have two array of different types share the same underlying memory, right? Otherwise a test like `testByteLong2` or `testOffHeapLong1` might vectorize with the assumption that we have two different memory regions, but they turn out to be the same, and we ignore the potential dependencies.
> 
> It would be really nice to thoroughly test things now. We found this bug by accident, and manual inspection. So obviously our tests are insufficient. We should do all sorts of combinations of array types and and various load/store types using `Unsafe`. And we should not just have IR rules, but also verify the results. But maybe we can only do that after we have also this cyclic dependency bug fixed https://github.com/openjdk/jdk/pull/12350, as discussed here https://github.com/openjdk/jdk/pull/12440#discussion_r1116622180. Then, we should also look into this issue https://github.com/openjdk/jdk/pull/12440#issuecomment-1439784711.
> 
> Why are we not finding these bugs quicker? The bugs are usually "wrong result" bugs, they do not hit any assert. So for fuzzing, you need to verify the output. And the fuzzer would also have to have `Unsafe` memory accesses implemented. I doubt that we have that currently. An other reason is that if we just do not vectorize, we just lose a bit of performance, and that is also difficult to notice (unless it is a drastic regression).

@eme64 thank you for answering my question about alias index.
And I agree we you and Roland that we can add cyclic dependency tests after you fix your bug. 
File RFE for new testing so we don't forget.

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

PR: https://git.openjdk.org/jdk/pull/12440