[PATCH] D38586: SLPVectorizer.cpp: Ensure SLPVectorizer can visit each block by dominated order

[NAKAMURA Takumi via Phabricator via llvm-commits]

chapuni added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp:3347
   // dominating it are visited.
-  std::stable_sort(CSEWorkList.begin(), CSEWorkList.end(),
-                   [this](const DomTreeNode *A, const DomTreeNode *B) {
-    return DT->properlyDominates(A, B);
-  });
+  // std::sort is unavailable since the comparator is asymmetric.
+  for (auto A = CSEWorkList.begin(), E = CSEWorkList.end(); A != E; ++A)
----------------
davide wrote:
> this is going to be stale once you have a value numbering to fix this in linear time.
The issues is not "stability", but properlyDominates(A,B) would inapplicable to sorting.


================
Comment at: llvm/trunk/test/Transforms/SLPVectorizer/X86/visit-dominated.ll:2
+; RUN: opt -slp-vectorizer < %s -S | FileCheck %s
+; Ensure the vectorizer can visit each block by dominated order.
+; VEC_VALUE_QUALTYPE should dominate others.
----------------
davide wrote:
> I'm not sure I understand this comment?
How about? "Ensure each dominator block comes first in advance of its users"


================
Comment at: llvm/trunk/test/Transforms/SLPVectorizer/X86/visit-dominated.ll:4
+; VEC_VALUE_QUALTYPE should dominate others.
+; QUAL1_*(s) may be inducted by VEC_VALUE_QUALTYPE, since their pred is "entry".
+
----------------
davide wrote:
> what do you mean by `may be inducted` ?
It might be superfluous. I'd like to explain why

  [[QUAL1:%.+]] = extractelement <2 x i64> [[VEC_VALUE_QUALTYPE]], i32 1

appears in a few place.


Repository:
  rL LLVM

https://reviews.llvm.org/D38586