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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev reopened this revision.
anton-afanasyev added a comment.
This revision is now accepted and ready to land.

> Looks like it crashes on shuffles, just need to add a check for shuffles in assert.

No, it crashes on `extractelement` with non-constant index. I've commented this inline.
The current crash could just be fixed by removing one assert check, but I've found several more minors, so may be it worth to revert? I think this should be recommited with negative tests for `extractelement` as wel as for `insertelement` (are we able to schedule `insertelement` with non-constant index now?)



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:221
+    return false;
+  if (isa<ExtractElementInst, ExtractValueInst>(I))
+    return isConstant(I->getOperand(1));
----------------
Btw, this second `ExtractValueInst` is redundant, it's checked above.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:234
+  if (all_of(VL, isVectorLikeInstWithConstOps))
+    return true;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5958
+  // instructions.
+  if (isa<PHINode>(S.OpValue) || isVectorLikeInstWithConstOps(S.OpValue))
     return nullptr;
----------------
`all_of(VL, isVectorLikeInstWithConstOps)` should be here instead?


================
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;
----------------
all_of(VL, isVectorLikeInstWithConstOps) should be here instead as well?


================
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 "
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108703