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

[Emanuel Peter]

On Thu, 9 Feb 2023 16:08:19 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> test/hotspot/jtreg/compiler/c2/irTests/TestVectorizationMismatchedAccess.java line 39:
>> 
>>> 37:  */
>>> 38: 
>>> 39: public class TestVectorizationMismatchedAccess {
>> 
>> Could you have a much simpler example, just with two int-arrays, where one allows vectorization and one does not. We still want there to be some vectorization. Maybe like this, maybe you have to play around with it a bit:
>> 
>> // make sure arr1 and arr2 are in different slices
>> for (int i ...) {
>>   arr1[i] = arr1[i-2] + 5; // do not vectorize, cyclic dependency
>>   arr2[i] = arr2[i] + 5; // should vectorize
>> }
>
> I agree that we should have such a test if we don't have one already but does it belong to that bug? That bug changes the behavior with mismatched accesses so it feels strange to add an unrelated test.` testByteByte1() `is a mismatched access test that vectorizes and that's unaffected by my change. I could add another one that doesn't vectorize because of a dependency with a similar mismatched access. What do you think?

I understand the code better now. I believe that two arrays of the same basic type never land in two separate memory sliced, but always the same. I discussed this with @rwestrel  a while ago. The reason is that EA does not give arrays their own slice, it determines the array to be NSR (not scalar replaceable) if it is ever used in a loop. That is of course a shame, and I hope we can work on this in the future (maybe have some analysis in SuperWord that refines the memory slices, and thus removes unnecessary dependencies). But as @rwestrel  says not the topic of this Bug.
The goal of this thread was to find an example where @rwestrel 's first commit made a worse decision than before. And I found another one after wards (https://github.com/openjdk/jdk/pull/12440#issuecomment-1425919095). @rwestrel now updated the fix, and this specific test should no longer fail.
@vnkozlov adding cyclic dependency tests before fixing https://github.com/openjdk/jdk/pull/12350 will be difficult. Maybe we need to fix both bugs first, and then once they are both integrated add more tests?

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

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