[PATCH] D136757: [SLP] Extend reordering data of tree entry to support PHI nodes

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3785
+    auto PHICompare = [](llvm::Value *V1, llvm::Value *V2) {
+      if (!V1->user_empty() && !V2->user_empty()) {
+        auto *FirstUserOfPhi1 = cast<Instruction>(*V1->user_begin());
----------------
dmgreen wrote:
> ABataev wrote:
> > Better to do:
> > ```
> > if (V1->user_empty() || V2->user_empty())
> >   return false;
> > ```
> This checks that V1 and V2 have some number of uses, but the code below then always looks at the first one, to see if it is an insert/extract. As far as I understand the order of use lists is not deterministic though, and we can't just rely on the first element always being the same item. It leads to non-determinism issues.
> 
> I hit this where clang was crashing whereas .ll files fed into opt were not (on the scalable vector issue @DavidSpickett mentioned). I'm not sure of the best way to fix this exactly, I've reverted the patch in 656f1d8b74df5d3f5f2bc75a5f2565df48340757. The SLP issue is hopefully easy enough to fix. There's a new test case in 0e9dfff37ef8f29cdda716d5ccb9d8e74d2a48fe.
> 
> https://godbolt.org/z/96axz5sbT and https://godbolt.org/z/e4WeTE7ve also have larger examples.
Yep, better to check for hasOneUse here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136757