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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a reviewer: dtemirbulatov.
spatel added a comment.

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?



================
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);
----------------
Can we add this assert independently of this patch?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:5754
+    }
+    // Mark reduction root as externally used.
+    ExternallyUsedValues[ReductionRoot];
----------------
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.


================
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
+
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54955