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

[Emanuel Peter]

On Thu, 2 Mar 2023 08:29:47 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Maybe we should first collect all legal packs and then try to find the best alignment, dropping packs that are unaligned on those platforms that support it.
>
> Hi @rwestrel @vnkozlov .
> 
> I have another general question. Should this really vectorize?
> 
>     @Test
>     @IR(counts = { IRNode.LOAD_VECTOR, ">=1", IRNode.STORE_VECTOR, ">=1" })
>     public static void testOffHeapLong1(long dest, long[] src) {
>         for (int i = 0; i < src.length; i++) {
>             UNSAFE.putLongUnaligned(null, dest + 8 * i, src[i]);
>         }
>     }
> 
> 
> I talked with @TobiHartmann about this. It is apparently possible to get the address of an array, and store in a `long`. In that case, we could play with that rawptr a bit, and create a cyclic dependency.
> 
> Pseudocode:
> 
> long[] arr = new long[1000];
> long ptr = unsafe.getTheArrayAddress(arr); // does not exist directly, but it is possible
> ptr += 8; // shift it one long forward
> testOffHeapLong1(ptr, arr);
> 
> 
> This should have different behavior if it is vectorized or not.
> 
> But maybe this is expected, and just a bad use of `Unsafe`. Probably we want this to vectorize, for the extra performance, and people who use `Unsafe` just have to be careful not to do such strange things.

> 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
}

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

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