[PATCH] D108703: [SLP]No need to schedule/check parent for extract{element/value} instruction.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:221
+    return false;
+  if (isa<ExtractElementInst, ExtractValueInst>(I))
+    return isConstant(I->getOperand(1));
----------------
anton-afanasyev wrote:
> Btw, this second `ExtractValueInst` is redundant, it's checked above.
No, it is required and checks for a different thing. It checks that the index is constant for such instructions.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:234
+  if (all_of(VL, isVectorLikeInstWithConstOps))
+    return true;
+
----------------
anton-afanasyev wrote:
> Why do we need this bypass? Looks like this check isn't related to `allSameBlock()`. One can imagine vector-like-insts with const ops in different BBs. We can explicitly do this check outside `allSameBlock()` wherever need.
Because we don't need to check the parents for such instructions. For InsertsElements it was checked already, for Undefs - no need, for Extracts with constant indices - not required.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5958
+  // instructions.
+  if (isa<PHINode>(S.OpValue) || isVectorLikeInstWithConstOps(S.OpValue))
     return nullptr;
----------------
anton-afanasyev wrote:
> `all_of(VL, isVectorLikeInstWithConstOps)` should be here instead?
No, not needed, just `isVectorLikeInstWithConstOps(S.OpValue)` is enough. Moreover, if we have inserts/extracts with non-const indices, they should be gathered, we can't vectorize them. Just added a special check in `BoUpSLP::buildTree_rec` function.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6054
                                                 Value *OpValue) {
-  if (isa<PHINode>(OpValue) || isa<InsertElementInst>(OpValue))
+  if (isa<PHINode>(OpValue) || isVectorLikeInstWithConstOps(OpValue))
     return;
----------------
anton-afanasyev wrote:
> all_of(VL, isVectorLikeInstWithConstOps) should be here instead as well?
Same as above


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6094
   assert(I && "bundle member must be an instruction");
-  assert(!isa<PHINode>(I) && !isa<InsertElementInst>(I) &&
-         "phi nodes/insertelements don't need to be scheduled");
+  assert(!isa<PHINode>(I) && !isVectorLikeInstWithConstOps(I) &&
+         "phi nodes/insertelements/extractelements/extractvalues don't need to "
----------------
anton-afanasyev wrote:
> We assert here for bundle member not being vector-like-inst-with-const-ops, but check only `S.OpValue` before in `tryScheduleBundle()`. Remove this new check?
No, the assert is correct, we should allow extracts with const indices to be scheduled. It is fixed by the mentioned additional check in `BoUpSLP::buildTree_rec`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108703