[PATCH] D87424: [SVE] Bail from VectorUtils heuristics for scalable vectors

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:554
+/// Given a mask vector of the form <Y x ty>, Return true if all of the
 /// elements of this predicate mask are true or undef.  That is, return true
+/// if all lanes can be assumed active.
----------------
efriedma wrote:
> If the input isn't i1, it isn't obvious what "true"/"false" means here.
I guess these functions make and don't check lots of assumptions. (element type is bool, type is not just int, etc...) I'm just trying to follow the boy scout code here and leave the code cleaner than I found it. It would be better if I just asserted that the mask type is a vector and the element type is a bool, then the function would actually do what it says on the tin.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:908
 /// vectors.  Is there something we can common this with?
-APInt llvm::possiblyDemandedEltsInMask(Value *Mask) {
+Optional<APInt> llvm::possiblyDemandedEltsInMask(Value *Mask) {
+  if (isa<ScalableVectorType>(Mask->getType()))
----------------
efriedma wrote:
> I think I'd prefer to handle this in the callers: if the vector is scalable, just don't call it.  Returning an Optional<> is more confusing, I think, since it can't fail for fixed vectors.
I can change it to assert for scalable vectors. If we ever add a notion of a scalable demanded elements (we do this thing with bitfields in lots of places), then it's possible that this function could return a value for scalable vectors. Though in that event the return type of this function would change anyways, so we might as well wait.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87424