[PATCH] D29641: [SLP] Fix for PR31847: Assertion failed: (isLoopInvariant(Operands[i], L) && "SCEVAddRecExpr operand is not loop-invariant!")

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a reviewer: hfinkel.
mkuper added a comment.

This more or less makes sense to me, although I'm not sure it's the right solution.
One other option would be to actually delete things on the fly, and solve the AliasCache option in a different way. (Why does the SLP vectorizer even have its own alias cache?)
Yet another, probably more realistic, option would be to keep the WeakVHs to solve the reallocation issue, but, actually "delete" everything we need to delete, instead of leaving hanging undefs.

Regardless, I believe Hans wants to merge this into 4.0 to fix PR31847. This doesn't look at all safe, and I don't feel confident enough in this to endorse it for the branch, without baking in-tree for a while. And another pair of eyes would be great.

Hal?



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:609
   /// Holds all of the instructions that we gathered.
-  SetVector<Instruction *> GatherSeq;
+  SetVector<InsertElementInst *> GatherSeq;
   /// A list of blocks that we are going to CSE.
----------------
Is this change (and the others turning instructions into insertelementinst) related?
If not, can they go into a separate patch?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2981
                   is_contained(UserIgnoreList, U)) &&
                  "Replacing out-of-tree value with undef");
         }
----------------
This comment no longer makes sense.
Does the whole check?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4504
+      // vector reductions (except for ReductionRoot node).
+      V.markForDeletion(
+          makeArrayRef(ReductionOps.begin(), std::prev(ReductionOps.end())));
----------------
Is there any interaction with the "extra values" stuff?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:5047
       // of having to reorder them later.
-      SetVector<Value *> Candidates(GEPList.begin(), GEPList.end());
+      SetVector<GetElementPtrInst *> Candidates(GEPList.begin(), GEPList.end());
 
----------------
Again, this is an unrelated change, right?


https://reviews.llvm.org/D29641