[PATCH] D131894: [SLP]Try to vectorize single store operands.

Tue Oct 31 15:29:35 PDT 2023

hoy added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12245
+        // Try to vectorize chain in store, if this is the only store to the
+        // address in the block.
+        if ((I == Stores.end() || I->second.size() == 1) &&
----------------
ABataev wrote:
> hoy wrote:
> > ABataev wrote:
> > > RKSimon wrote:
> > > > ABataev wrote:
> > > > > RKSimon wrote:
> > > > > > 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?
> > > > > 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. 
> > > > 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.
> > > Sure, will do.
> > 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.
> 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.
I see. It looks like `ShouldStartVectorizeHorAtStore` can be overwritten by the checks here, even if it is on. Is it expected?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131894/new/

https://reviews.llvm.org/D131894