Fix SLP score for out of order contiguous loads

[alban.bridonneau]

Summary

SLP uses the distance between pointers to optimize
the getShallowScore. However the current code misses
the case where we are trying to vectorize for VF=4, and the distance
between pointers is 2. In that case the returned score
reflects the case of contiguous loads, when it's not actually
contiguous.

The attached unit tests have 5 loads, where the program order
is not the same as the offset order in the GEPs. So, the choice
of which 4 loads to bundle together matters. If we pick the
first 4, then we can vectorize with VF=4. If we pick the
last 4, then we can only vectorize with VF=2.

This patch makes a more conservative choice, to consider
all distances>1 to not be a case of contiguous load, and
give those cases a lower score.

[ABataev]

Try to simplify the tests.

[alban.bridonneau]

I have simplified the unit tests.
As far as this patch goes, both tests were checking the same
behaviour, so I have only kept one. The complex data structures
and unnecesary attributes have been removed, and the IR has been reordered
to make the patterns clearer. Let me know if further reduction
is needed.

[vporpo]

llvm/test/Transforms/SLPVectorizer/AArch64/tsc-s116.ll

18–24

This test should work fine without the loop and the entry and for.cond.cleanup blocks. Please remove these extra blocks and the loop-related code.

26–29

Do you think you could replace these offset calculations with constants? This would make the test a lot smaller.

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

1199

Actually, this code is part of non-power-of-2 vectorization support in SLP, the problem should disappear once it is fully landed.

[alban.bridonneau]

@vporpo I was just starting to look at removing the loop-related code. That's a good point, i keep thinking about SLP in the context of loops, but it shouldn't be just about loops. The various costs seem completely different, it might require some extra work to reproduce the exact case without the loop control code.

@ABataev , that's interesting. Is that a patch you are working on? I'd be curious to take a look if that's possible.

[ABataev]

@ABataev , that's interesting. Is that a patch you are working on? I'd be curious to take a look if that's possible.

D57059 but I did not update it for a while + D105986, which is part of the first one.

[alban.bridonneau]

Further reduction of the unit test

[alban.bridonneau]

@ABataev , I had a look at the various patches you pointed. That looks like a proper piece of work! Given how much change is going to happen, i don't think it is worth pushing the fix i am proposing today.

However, I'd like to make sure my test case will get fixed, if possible. I tried your original patch (rebased last July) on the unit test and that failed. Do you think you could incorporate this unit test in your work?

I have reduced the unit test further, there is no more loop control, and the gep indices have been cleaned up. The CHECK lines are also more targeted to say we are looking for a vectorization of 4 (it currently does 2). We can actually see much better now which is the root cause of my issue. The LHS operands of the fmul are what's deciding which is the first group of loads beeing evaluated, and that group does not have 4 contiguous loads.

[vporpo]

Thanks for updating the test, it looks much better now :)
I agree, I don't think that this needs to wait for the non-power-of-2 support, unless @ABataev has some objection.

llvm/test/Transforms/SLPVectorizer/AArch64/tsc-s116.ll

7–10

Please use the utils/update_test_checks.py script for generating the checks. It makes it easier to diff test failures when tests contain automatically generated checks.

[ABataev]

In D123516#3451025, @alban.bridonneau wrote:

@ABataev , I had a look at the various patches you pointed. That looks like a proper piece of work! Given how much change is going to happen, i don't think it is worth pushing the fix i am proposing today.

However, I'd like to make sure my test case will get fixed, if possible. I tried your original patch (rebased last July) on the unit test and that failed. Do you think you could incorporate this unit test in your work?

Sure, +1 here. You can add the test separately with the CHECKs for currently emitted code and a FIXME message how it is expected to be.
The more tests we have, the better. Iy would be much easier for me to tune the patch.

I have reduced the unit test further, there is no more loop control, and the gep indices have been cleaned up. The CHECK lines are also more targeted to say we are looking for a vectorization of 4 (it currently does 2). We can actually see much better now which is the root cause of my issue. The LHS operands of the fmul are what's deciding which is the first group of loads beeing evaluated, and that group does not have 4 contiguous loads.

I'll check it, thanks. Follow suggestions from @vporpo to commit the test.

[alban.bridonneau]

The CHECK lines are now autogenerated.

I also removed the code change, and instead added
a FIXME with details of what that unit test should
be checking.

[ABataev]

LG

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

 //===- SLPVectorizer.cpp - A bottom up SLP Vectorizer ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
static int getShallowScore(Value *V1, Value *V2, const DataLayout &DL,
 
         Optional<int> Dist = getPointersDiff(
             LI1->getType(), LI1->getPointerOperand(), LI2->getType(),
             LI2->getPointerOperand(), DL, SE, /*StrictCheck=*/true);
         if (!Dist || *Dist == 0)
           return VLOperands::ScoreFail;
         // The distance is too large - still may be profitable to use masked
         // loads/gathers.
-        if (std::abs(*Dist) > NumLanes / 2)
+        if (std::abs(*Dist) > 1)
           return VLOperands::ScoreAltOpcodes;
         // This still will detect consecutive loads, but we might have "holes"
         // in some cases. It is ok for non-power-2 vectorization and may produce
         // better results. It should not affect current vectorization.
         return (*Dist > 0) ? VLOperands::ScoreConsecutiveLoads
                            : VLOperands::ScoreReversedLoads;
       }
 

llvm/test/Transforms/SLPVectorizer/AArch64/tsc-s116.ll

+; RUN: opt < %s -slp-vectorizer -S -mtriple=aarch64-unknown-unknown | FileCheck %s
+
+; This test is reduced from the TSVC evaluation of vectorizers:
+; https://github.com/llvm/llvm-test-suite/commits/main/MultiSource/Benchmarks/TSVC/LoopRerolling-flt/tsc.c
+
+define void @s116_modified(float* %a) {
+; CHECK-LABEL: @s116_modified(
+; CHECK: load <4 x float>, <4 x float>*
+; CHECK: fmul fast <4 x float>
+; CHECK: store <4 x float>
+  %gep0 = getelementptr inbounds float, float* %a, i64 0
+  %gep1 = getelementptr inbounds float, float* %a, i64 1
+  %gep2 = getelementptr inbounds float, float* %a, i64 2
+  %gep3 = getelementptr inbounds float, float* %a, i64 3
+  %gep4 = getelementptr inbounds float, float* %a, i64 4
+  %ld0 = load float, float* %gep0
+  %ld1 = load float, float* %gep1
+  %ld2 = load float, float* %gep2
+  %ld3 = load float, float* %gep3
+  %ld4 = load float, float* %gep4
+  %mul0 = fmul fast float %ld0, %ld1
+  %mul1 = fmul fast float %ld2, %ld1
+  %mul2 = fmul fast float %ld3, %ld2
+  %mul3 = fmul fast float %ld4, %ld3
+  store float %mul0, float* %gep0
+  store float %mul1, float* %gep1
+  store float %mul2, float* %gep2
+  store float %mul3, float* %gep3
+  ret void
+}
+
+