In D129008#3634073, @jdoerfert wrote:

cleanup/destructor test missing.

@ABataev WDYT?

Reworked addition of the reduction. For logical ops, reduction result is added as a first operand,
it is safe to use it as an LHS in scalar logical ands/ors.
For others, it is added as a last operand.

LG

Fixes and improvements.

LG

In D107966#3620735, @vdmitrie wrote:

reduced.ll12 KBDownload

patch.txt1 KBDownload

There is another issue which I tracked down to this patch but it is kind of hidden. In order to reveal the issue please apply attached patch ( that is basically enabling expensive checks and added verifyFunction right after vectorized code generated.

Crash looks like this:
Instruction does not dominate all uses!

%41 = insertelement <4 x i32> %40, i32 %32, i32 1
%39 = insertelement <4 x i32> %41, i32 poison, i32 2

opt: /path/llvm-project/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8404: llvm::Value* llvm::slpvectorizer::BoUpSLP::vectorizeTree(): Assertion `!verifyFunction(*F, &dbgs()) && "Broken after vec"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0. Program arguments: bin/opt -slp-vectorizer -mcpu=skylake -disable-output reduced.ll

LG

In D107966#3620735, @vdmitrie wrote:

reduced.ll12 KBDownload

patch.txt1 KBDownload

There is another issue which I tracked down to this patch but it is kind of hidden. In order to reveal the issue please apply attached patch ( that is basically enabling expensive checks and added verifyFunction right after vectorized code generated.

Crash looks like this:
Instruction does not dominate all uses!

%41 = insertelement <4 x i32> %40, i32 %32, i32 1
%39 = insertelement <4 x i32> %41, i32 poison, i32 2

opt: /path/llvm-project/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8404: llvm::Value* llvm::slpvectorizer::BoUpSLP::vectorizeTree(): Assertion `!verifyFunction(*F, &dbgs()) && "Broken after vec"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0. Program arguments: bin/opt -slp-vectorizer -mcpu=skylake -disable-output reduced.ll

LG

LG

Can you accept the patch?

Address comments.

LG

In D115462#3613570, @akhuang wrote:

I saw another crash on chromium builds - not sure if this is related to the previous mentioned crash but here's a creduced repro:

struct f {
  float g;
  float h;
};
struct j {
  j() = default;
  j(float k, float l) : c(k), d(l) {}
  j operator*(j k) const { return j(k.a + c, b + k.d); }
  float a = 1.0f;
  float b = 0.0f;
  float c = 0.0f;
  float d = 1.0f;
  float e = 0.0f;
};
struct m {
  j n() const;
  f o;
  j p;
};
j m::n() const {
  if (o.g || o.h)
    return j();
  j a;
  return p * a;
}

build with

clang -cc1 -O2 -vectorize-slp -emit-llvm -fno-delete-null-pointer-checks t.cpp

Address comments

Address comment

Rebase

Added a fix for gathers reordering

LG

In D115462#3613570, @akhuang wrote:

I saw another crash on chromium builds - not sure if this is related to the previous mentioned crash but here's a creduced repro:

struct f {
  float g;
  float h;
};
struct j {
  j() = default;
  j(float k, float l) : c(k), d(l) {}
  j operator*(j k) const { return j(k.a + c, b + k.d); }
  float a = 1.0f;
  float b = 0.0f;
  float c = 0.0f;
  float d = 1.0f;
  float e = 0.0f;
};
struct m {
  j n() const;
  f o;
  j p;
};
j m::n() const {
  if (o.g || o.h)
    return j();
  j a;
  return p * a;
}

build with

clang -cc1 -O2 -vectorize-slp -emit-llvm -fno-delete-null-pointer-checks t.cpp

LG

In D115462#3613388, @manojgupta wrote:

Still crashes on trunk.

C-reduce Test case:

typedef __INT64_TYPE__ int64_t;
int sbr_autocorrelate_c_x_i;
void phiautocorr_calc(int64_t );

void sbr_autocorrelate_c_x(void) {
  int(*x)[2] = sbr_autocorrelate_c_x;
  int64_t accu_re , accu_im = 0;
  for (; sbr_autocorrelate_c_x_i; sbr_autocorrelate_c_x_i++) {
    accu_re +=
        x[sbr_autocorrelate_c_x_i][0] * x[sbr_autocorrelate_c_x_i + 2][0];
    accu_re +=
        x[sbr_autocorrelate_c_x_i][1] * x[sbr_autocorrelate_c_x_i + 2][1];
    accu_im +=
        x[sbr_autocorrelate_c_x_i][0] * x[sbr_autocorrelate_c_x_i + 2][1];
    accu_im -=
        x[sbr_autocorrelate_c_x_i][1] * x[sbr_autocorrelate_c_x_i + 2][0];
  }
  phiautocorr_calc(accu_im);
  phiautocorr_calc(accu_re);
}

Crashes with
clang -Os -c test.c --target=armv7a-linux-gnueabihf -mfpu=neon -Wno-error

llvm/llvm-project/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7744: llvm::Value *llvm::slpvectorizer::BoUpSLP::createBuildVector(ArrayRef<llvm::Value *>): Assertion `any_of(VectorizableTree, [VL](const std::unique_ptr<TreeEntry> &TE) { return TE->State == TreeEntry::NeedToGather && TE->isSame(VL); }) && "Non-matching gather node."' failed.

In D115462#3611255, @manojgupta wrote:

Heads up: I am seeing a clang crash on arm with this commit:

commit 2faacf61a50e7f23fd10927cbbb98c59799bfcd0
Author: Alexey Bataev <a.bataev@outlook.com>
Date: Thu Dec 9 10:34:08 2021 -0800
CommitDate: Fri Jun 24 09:28:01 2022 -0700

[SLP]Improve shuffles cost estimation where possible.

I am trying to create a reduced test case.

Address comments

Rebase

In D115462#3607097, @MaskRay wrote:

I more firmly believe I made a mistake. Sorry for that. You may recommit.

LG

In D115462#3606101, @MaskRay wrote:

In D115462#3605808, @ABataev wrote:

In D115462#3603899, @MaskRay wrote:

Here is -mllvm -print-changed -mllvm -print-module-scope output before SLPVectorizer: https://gist.github.com/MaskRay/23f0db50e136127fda1b4f83db2488da

Double checked the patch and the results. The patch itself does not change the vectorization, just adjusts the cost model estimation. Some of the buildvector sequences are not profitable for re-vectorization after this patch, nothing else. Plus, the code after this patch results in the same code, being applied to the compiler without the patch, i.e. the code transformations are the same, just different vectorization.

I have added IR before/after SLPVectorizer, with and without the two commits, and generated assembly to https://gist.github.com/MaskRay/23f0db50e136127fda1b4f83db2488da
Hope they are useful.

Maybe the debug info is corrupted somehow? Or there are other effects?

The codegen is corrupted. I provide debug info just in case it helps analyze the problem.

In D115462#3603899, @MaskRay wrote:

Here is -mllvm -print-changed -mllvm -print-module-scope output before SLPVectorizer: https://gist.github.com/MaskRay/23f0db50e136127fda1b4f83db2488da

LG

In D125669#3604302, @RitanyaB wrote:

In D125669#3599134, @ABataev wrote:

What about the description? Shall we still emit if(0)?

Can you please elaborate for more clarity?

LG

Removed getConstantBuildVectorCost, the analysis for constant values already exists in getArithmeticInstrCost. Added support for const operand for stores in getMemoryOpCost function.

In D127531#3602270, @vdmitrie wrote:

In D127531#3602238, @ABataev wrote:

In D127531#3602215, @vdmitrie wrote:

I'm not sure about how the test case affected supposed to execute the changes.
Could you please consider adding more tests to be clearer about the patch purpose?

It is for compile time mostly, nothing else. Just allows to detect gathered elements before scheduling.

Yes, but the summary also says more vector instructions generated. What specific cases are handled now but failed previously?

In D127531#3602215, @vdmitrie wrote:

I'm not sure about how the test case affected supposed to execute the changes.
Could you please consider adding more tests to be clearer about the patch purpose?

Ping!

Ping!

Ping!

Ping!

What about the description? Shall we still emit if(0)?

Adjust OffsetBeg for better shuffle mask estimation.

Address comments

provide full context of the changes.

Why do we need to insert new implicit DSA? Only explicit DSAs are expected to be stored, implicit ones can be deduced using the rules.

LG

Rebase

Rebase

Fixed order of checks.

Address comments

Improve checks to avoid regressions