This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Try to vectorize single store operands.
ClosedPublic

Authored by ABataev on Aug 15 2022, 8:05 AM.

Details

Summary

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.

Diff Detail

Event Timeline

ABataev created this revision.Aug 15 2022, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 8:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
ABataev requested review of this revision.Aug 15 2022, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 8:05 AM

Does this fix https://github.com/llvm/llvm-project/issues/51320 and if so could you add any missing test coverage please?

Does this fix https://github.com/llvm/llvm-project/issues/51320 and if so could you add any missing test coverage please?

Yep, it fixes this issue, will add a test.

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.

ABataev updated this revision to Diff 452770.Aug 15 2022, 12:07 PM

Update description + rebase

RKSimon added inline comments.Aug 16 2022, 5:10 AM
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?

ABataev added inline comments.Aug 16 2022, 5:26 AM
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.
The alternative solution is just enable ShouldStartVectorizeHorAtStore by default but it requires some extra work with side effect and compile time analysis.

RKSimon accepted this revision.Aug 16 2022, 5:32 AM

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.

This revision is now accepted and ready to land.Aug 16 2022, 5:32 AM
ABataev added inline comments.Aug 16 2022, 5:35 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
12246

Sure, will do.

This revision was automatically updated to reflect the committed changes.
hoy added a subscriber: hoy.Oct 31 2023, 2:59 PM
hoy added inline comments.
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.

ABataev added inline comments.Oct 31 2023, 3:12 PM
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.

hoy added inline comments.Oct 31 2023, 3:29 PM
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?

ABataev added inline comments.Oct 31 2023, 3:38 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
12246

Yes, need to use |=

hoy added inline comments.Oct 31 2023, 4:49 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
12246

Can we just make the change now, as ShouldStartVectorizeHorAtStore defaults to off?

ABataev added inline comments.Oct 31 2023, 4:58 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
12246

Will fix it tomorrow

ABataev added inline comments.Nov 1 2023, 8:42 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
12246
hoy added inline comments.Nov 1 2023, 10:01 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
12246

Thanks for the quick fix!