[PATCH] D49428: [LSV] Look through selects for consecutive addresses

Roman Tereshin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 24 16:00:23 PDT 2018


rtereshin added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:440-441
+
+  if (auto *SelectA = dyn_cast<SelectInst>(PtrA))
+    if (auto *SelectB = dyn_cast<SelectInst>(PtrB))
+      return SelectA->getCondition() == SelectB->getCondition() &&
----------------
arsenm wrote:
> rtereshin wrote:
> > rtereshin wrote:
> > > arsenm wrote:
> > > > Braces
> > > Where exactly would you like braces? First level if, second level if, or both?
> > > 
> > > I believe no curly braces around 1 statement bodies of if/then/else/for/while/do-while statements is the standard LLVM code style.
> > Going with braces around second level if for now.
> I would put both, as both are surrounding multi-line things
Done.


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:674-679
+static ChainID getChainID(const Value *Ptr, const DataLayout &DL) {
+  const Value *ObjPtr = GetUnderlyingObject(Ptr, DL);
+  if (const auto *Sel = dyn_cast<SelectInst>(ObjPtr))
+    return Sel->getCondition();
+  return ObjPtr;
+}
----------------
arsenm wrote:
> rtereshin wrote:
> > arsenm wrote:
> > > I'm a bit confused by this ChainID concept. Why is the select condition used for this and not the select itself?
> > > I'm a bit confused by this ChainID concept.
> > 
> > `ChainID` is an arbitrary token that is allowed to be different only for the accesses that are guaranteed to be non-consecutive. It's used (and it's predecessor was used) for grouping instructions together and reducing the number of instructions the main search operates on at a time, i.e. this is to reduce compile time and nothing else as the main search has (and had) `O(n^2)` time complexity.
> > 
> > The exact way of determining accesses that are guaranteed to be non-consecutive does not affect the rest of the pass as soon as it doesn't return different `ChainID`s for consecutive accesses. I played with a few different ones and I expect it to change in the future. All one needs to do to change it is to change the type definition and the `getChainID` function implementation. In particular, returning the same `ChainID` for every `Ptr` works too, but increases compile time.
> > 
> > Of course, "guaranteed to be non-consecutive" above means "guaranteed to be classified as non-consecutive by isConsecutiveAccess method".
> > 
> > > Why is the select condition used for this and not the select itself?
> > The select's themselves are distinct instructions even if they share the same condition and evaluate to consecutive pointers for true and false values of the condition. Therefore using the select's themselves for grouping instructions would put consecutive accesses into different lists and they wouldn't be even checked for being consecutive, and won't be vectorized. Which defies the purpose of this patch.
> This should be in a comment somewhere
Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D49428





More information about the llvm-commits mailing list