[PATCH] D54955: [SLP]Fix PR39774: Set ReductionRoot if the original instruction is vectorized.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked 3 inline comments as done.
ABataev added a comment.

In D54955#1310191 <https://reviews.llvm.org/D54955#1310191>, @spatel wrote:

> This makes sense to me, but I'm not very familiar with SLP, so hopefully we can get a 2nd opinion.
>
> By making the reduction root externally used, does that mean that some other pass is now responsible for deleting it (assuming it is not actually externally used) where before SLP would delete it?


No, `ReductionRoot` is deleted then at the end of the reduction vectorization, see line 5844. It is marked as externally used by the future reduction vectorization process.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:5751
+    for (auto &Pair : ExtraArgs) {
+      assert(Pair.first && "DebugLoc must be set.");
       ExternallyUsedValues[Pair.second].push_back(Pair.first);
----------------
spatel wrote:
> Can we add this assert independently of this patch?
I think no, because this is the replacement of the deleted assert on line 5829.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:5754
+    }
+    // Mark reduction root as externally used.
+    ExternallyUsedValues[ReductionRoot];
----------------
spatel wrote:
> Better to have this code comment explain why this is necessary:
>   // The reduction root is used as the insertion point for new instructions, 
>   // so set it as externally used to prevent it from being deleted.
Ok, will do.


================
Comment at: test/Transforms/SLPVectorizer/X86/PR39774.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -slp-vectorizer -S < %s -mtriple=x86_64-unknown-linux-gnu -mcpu=skylake -slp-threshold=-7 | FileCheck %s
+
----------------
spatel wrote:
> We do not need to specify triple, cpu, or slp-threshold to have this crash currently, right? If this makes the test more robust against future cost model changes, that's good. If not, we could remove those?
Well, currently we need to specify all that stuff to reproduce the crash. With the updated cost model, the slp-threshold can be removed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54955