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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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() &&
----------------
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


================
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;
+}
----------------
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


Repository:
  rL LLVM

https://reviews.llvm.org/D49428