[PATCH] D70068: [SLP] Enhance SLPVectorizer to vectorize vector aggregate

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2879-2880
+  if (auto *VT = dyn_cast<VectorType>(EltTy)) {
+    if (VT->isScalable())
+      return 0;
+    EltTy = VT->getElementType();
----------------
anton-afanasyev wrote:
> anton-afanasyev wrote:
> > RKSimon wrote:
> > > anton-afanasyev wrote:
> > > > ABataev wrote:
> > > > > anton-afanasyev wrote:
> > > > > > ABataev wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Why do we have this check?
> > > > > > > Also, would be good to have a test for this if we don't have it yet.
> > > > > > Hmm, you're right, I'm just to delete this check. I haven't faced any issue concerning scalability, but decided to check it just in case (`{ <vscale x 2 x float>, <vscale x 2 x float> }` is not isomorphic to `<4 x float>`?). But I see no similar check across `SLPVectorizer.cpp`, so I'm to delete this check.
> > > > > My question was different. I just wanted some explanation what's the problem with the scalable vectors? If the check is required we must have it. Also, still, would be good to have a test for this situation.
> > > > I wasn't familiar with `vscale` property, but learned it now. That check was actually unnecessary: `{ <vscale x N x Ty>, <vscale x N x Ty> }` maps to `<vscale x 2N x Ty>`,  so we can deal with this scalability set to true.
> > > Probably worth adding at least some test coverage for this - in aarch64 maybe?
> > Actually `vscale` property cannot be set to true for such cases.
> > The structure like `{ <vscale x 2 x float> }` is forbidden, I've got `error: invalid element type for struct` trying to `opt`  this.
> What do you mean by test coverage here? I can copy the same `pr42022.ll` to `llvm/test/Transforms/SLPVectorizer/AArch64`, but the only difference will be `-mtriple=` option.
> If you are talking about original issue (pointed here https://llvm.org/pr42022), for aarch64 the vectorization is done for "Bad" case and is failed for two "Good" case (https://godbolt.org/z/5JEGSm), but the root cause is different ABI of aarch64 compared to x86-64. This patch doesn't change this and doesn't concern this.
I was referring to adding vscale tests - but if they can't occur then that's fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70068