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

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:140
 
+  enum { RecursionLimit = 3 };
+
----------------
arsenm wrote:
> Why is this an enum?
Sure, will change that to `static const` or `static constexpr`. Which one would you prefer?


================
Comment at: lib/Transforms/Vectorize/LoadStoreVectorizer.cpp:315
+bool Vectorizer::areConsecutivePointers(Value *PtrA, Value *PtrB,
+                                        APInt PtrDelta, unsigned MaxRecurse) {
   unsigned PtrBitWidth = DL.getPointerTypeSizeInBits(PtrA->getType());
----------------
arsenm wrote:
> Const method?
Maybe, I never checked. Most likely not, it calls methods on `ScalarEvolution &SE`, which is part of the object, and those methods change the SCEVs' cache that is owned by `SE`, so most likely they aren't const methods themselves.

If it is a const method though, will do.


================
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:
> 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.


================
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:
> 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D49428