[PATCH] D89969: [SLP] Consider alternatives for cost of select instructions.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3559-3595
+            auto *Select = dyn_cast<SelectInst>(I);
+            if (!Select)
+              return false;
+            auto *Cmp = dyn_cast<CmpInst>(Select->getOperand(0));
+            if (!Cmp)
+              return false;
+
----------------
dmgreen wrote:
> ABataev wrote:
> > Use `match()` functions to match the pattern. Also, what's the criterion for generating intrinsic or vector instruction? Can we rely on it unconditionally and just choose the minimum cost?
> There's something called matchSelectPattern that might help quite a bit too.
> . Also, what's the criterion for generating intrinsic or vector instruction? Can we rely on it unconditionally and just choose the minimum cost?

Currently for costing, it just picks whatever has the cheaper cost (select lowering or min/max version). The code generated by the SLP vectorizer is currently unchanged and it relies on the backend to pick whichever version is more profitable. 

This works well on AArch64, but there could be a case where for example SLP vectorizer decides it is only profitable with min/max, but the backend is missing a corresponding fold and won't be able to generate the right instructions.

We could add a general fold to VectorCombine as follow up, to behave more predictable, if that is desired, or adjust the SLP codegen.

I am also planing a follow-up that supports additional select patterns that can be lowered more efficiently than currently on AArch64 that are not min/max.

> There's something called matchSelectPattern that might help quite a bit too.

That's useful, thanks! Updated the code to use that. Unfortunately it seems like we still need some code to turn the pattern into an intrinsic, but maybe the code should be moved to SelectPatternResult?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89969