[PATCH] D132949: [SLP]Fix PR57447: Assertion `!getTreeEntry(V) && "Scalar already in tree!"' failed.

Tue Aug 30 11:19:29 PDT 2022

vdmitrie added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4769
     auto *I = dyn_cast<Instruction>(V);
-    if (!I)
+    if (!I && (!UserTreeIdx.UserTE ||
+               UserTreeIdx.UserTE->State != TreeEntry::ScatterVectorize))
----------------
vdmitrie wrote:
>  "!I" can be replaced with "!isa<Instruction>(V)"
> We already have few places of the same pattern addressing specifically the operand of a ScatterVectorize node: " UserTreeIdx.UserTE &&  UserTreeIdx.UserTE->State == TreeEntry::ScatterVectorize".
> We also define isScatterUser variable at line 5221 with the same value.
> I'd suggest to pull the variable definition up and use it instead of the repeating pattern.
> The comment before the loop is probably worth updating too.
A side note. IMO terminology used here in SLP vectorizer wrt vectorizing non-unit stride loads is really confusing as word "scatter" is typically associated with stores. But I'm not about changing the naming in context of this PR. What I wanted to highlight is that IsScatterUser is also not the best choice for the variable name. It is even more confusing because it sounds like "this VL is a user of ScatterVectorizer node" (which by the way may not have users as loads are terminators) rather than "user of this VL is ScatterVectorize node". It would be nice if you come up with better name for the variable (preferably not having "scatter" in its name).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132949