Currently, we try to vectorize values, feeding into stores, only if
slp-vectorize-hor-store option is provided. We can safely enable
vectorization of the value operand of a single store in the basic block,
if the operand value is used only in store.
It should enable extra vectorization and should not increase compile
time significantly.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Does this fix https://github.com/llvm/llvm-project/issues/51320 and if so could you add any missing test coverage please?
test/Transforms/SLPVectorizer/X86/horizontal.ll already has the tests for this issue, actually.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | This comment suggests we're checking for aliasing of the pointer as well? But I thought this was supposed to be purely about the stored value having one use? |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | I tried to reduce compile time effect with this change, so added another check that we have only single store in the basic block for the provided base pointer. |
LGTM with one minor
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | OK - please can you make it clear in the comment that this is purely to reduce compile time and that ShouldStartVectorizeHorAtStore could one day be switched on by default. |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | Sure, will do. |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | How much compile-time overhead did you see without I->second.size() == 1 change? I'm seeing a need to unblock this. For example, unrolling a loop may cause a store instruction duplicated multiple times and horizontal-vectorizing the duplicated stores are important. Also I'm wondering if adding an optional upper bound through command line is reasonable. Thanks. |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | I did not measure it, just disabled it for the sake of the ShouldStartVectorizeHorAtStore option. Requires some extra investigation of the compile time and perf effects. |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | I see. It looks like ShouldStartVectorizeHorAtStore can be overwritten by the checks here, even if it is on. Is it expected? |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | Yes, need to use |= |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | Can we just make the change now, as ShouldStartVectorizeHorAtStore defaults to off? |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | Will fix it tomorrow |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 |
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
12246 | Thanks for the quick fix! |
This comment suggests we're checking for aliasing of the pointer as well? But I thought this was supposed to be purely about the stored value having one use?