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

[Roland Westrelin]

On Thu, 2 Mar 2023 17:30:06 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>>> Thanks for the comments @eme64 @vnkozlov Looking at the code again, if `vectors_should_be_aligned()` is true, if `create_pack` is false, the current code removes every memops and already created packset with `same_velt_type()` true: That can't be motivated by a correctness issue. So I suppose we want to preserve that behavior. Wouldn't we need the change of the last commit I pushed then?
>> 
>> I think the reason we used `same_velt_type` was that we were confused. Or maybe we did that before we had memory slices, and using `same_velt_type` was at least already an improvemnt? At any rate: it was confused and leads to Bugs in conjunction with `Unsafe`, as my example showed.
>> 
>> Keeping `same_velt_type` will probably not harm much, but be more restrictive than neccessary.
>> It will not harm much because `velt_type == memory_slice` as long as we are not using `Unsafe`.
>> And when we do use `Unsafe`, we probably do not use it in very wild ways.
>> 
>> One "wild" use might be something like this:
>> 
>> void test(int[] iarr, float[] farr) {
>>     // cyclic dependency -> not vectorized
>>     in v1 = (int)Unsafe.LoadF(iarr, i);  // assume this to be best
>>     Unsafe.StoreI(iarr, i + 1);
>>     // separate slice -> could be vectorized
>>     Unsafe.StoreI(farr, i) = Unsafe.LoadI(farr, i);   // on different slice as best, but have same velt_type -> rejected
>>     // We end up vectorizing nothing, even though we could vectorize the farr
>> }
>
>> I think the reason we used `same_velt_type` was that we were confused. Or maybe we did that before we had memory slices, and using `same_velt_type` was at least already an improvemnt? At any rate: it was confused and leads to Bugs in conjunction with `Unsafe`, as my example showed.
> 
> I did not consider using memory slices (or unsafe access) when worked on this code. Same element type was easy choice for this check.
> 
>> I also don't think we should spend too much time making sure every possible combinations of unsafe accesses optimize well or even correctly if it's too much work. Once people start using unsafe, they are on their own. I think we should stick with whatever feels reasonable or is used in the core libraries (hopefully the second category is included in the first category).
> 
> Yes, even without vectorization we can construct a Java test with Unsafe access which has cyclic dependencies and overwrite elements intentionally or by mistake. I remember one such case in system libraries several years ago which was fixed.
> 
> JIT optimization should not introduce wrong behavior  if Java code does not have it. But if we can correctly detect and reject cyclic dependency we can vectorize it. Original Roland's example and last Emanuel's `StoreI(farr, i)` example don't have "bad" cyclic dependency - at worst they store the same values to the same elements. So it is all about cyclic dependency detection and assumption that we may accessing the same array.

Thanks for the reviews, testing and discussion @vnkozlov @eme64 
I updated the test case once more:
- Some of the ByteByte test were broken
-  I made sure the test only runs on aarch64 and x64, the platforms I can test this on. Also the test is now skipped if `AlignVector` is false
- I added verification code for the content of the arrays

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

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