[PATCH] D81500: [SVE] Remove calls to VectorType::getNumElements from IR

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked 8 inline comments as done.
ctetreau added inline comments.


================
Comment at: llvm/include/llvm/IR/MatrixBuilder.h:47
       LHS = B.CreateVectorSplat(
-          cast<VectorType>(RHS->getType())->getNumElements(), LHS,
+          cast<FixedVectorType>(RHS->getType())->getNumElements(), LHS,
           "scalar.splat");
----------------
ctetreau wrote:
> RKSimon wrote:
> > RKSimon wrote:
> > > Do we not have a method for creating splat vectors that works for fixed and non-fixed vector types?
> > I think CreateVectorSplat needs to be altered to take a ElementCount type instead of a raw unsigned elt count - are you in a position to do this first?
> The change to CreateVectorSplat is actually fairly trivial. ElementCount can just be used in place of an unsigned within its body and signature. I can certainly make the change, but I'd prefer to do it separate from this patch because:
> 
> 1) it's a behavior change that needs tests
> 2) as far as I can tell, none of this matrix stuff supports scalable vectors, so making this change would have low practical value
I'm going to go ahead and use your new overload that takes an ElementCount. I'm going to assert that the vector is not scalable though to ensure that this is NFC.


================
Comment at: llvm/lib/IR/Constants.cpp:233
     return false;
-  for (unsigned i = 0, e = VTy->getNumElements(); i != e; ++i) {
+  for (unsigned i = 0, e = cast<FixedVectorType>(VTy)->getNumElements(); i != e;
+       ++i) {
----------------
RKSimon wrote:
> I think you can change the VTy cast without changing functionality here.
ConstantAggregateZero and UndefValue can have ScalableVectorType, but since this function explicitly returns false for 0, and the cast will fail for UndeValue, also producing false, I agree with your statement.

I went ahead and changed the comment to mention it only returns true for fixed width vectors


================
Comment at: llvm/lib/IR/Constants.cpp:310
   if (auto *VTy = dyn_cast<VectorType>(getType())) {
-    for (unsigned i = 0, e = VTy->getNumElements(); i != e; ++i)
+    for (unsigned i = 0, e = cast<FixedVectorType>(VTy)->getNumElements();
+         i != e; ++i)
----------------
RKSimon wrote:
> I think you can change the VTy cast without changing functionality here.
this function previously returned true if the entire vector was undef, and dyn_cast'ing to FixedVectorType will change this behavior. I will make this function robust to this case though, and update the comment to mention that this is the one case where this will return true for scalable vector constants

I guess technically, getAggregateElement for a scalable UndefValue asserts, but for the purposes of this function, it all should work out since the caller isn't providing an index, and UndefValue :: <vscale x n x ty> does actually contain UndefValue.


================
Comment at: llvm/lib/IR/Constants.cpp:321
   if (auto *VTy = dyn_cast<VectorType>(getType())) {
-    for (unsigned i = 0, e = VTy->getNumElements(); i != e; ++i)
+    for (unsigned i = 0, e = cast<FixedVectorType>(VTy)->getNumElements();
+         i != e; ++i)
----------------
RKSimon wrote:
> I think you can change the VTy cast without changing functionality here.
Since this will never actually return true for a scalable vector, I agree. I'll update the comment document that it only returns true for fixed width vectors


================
Comment at: llvm/lib/IR/Function.cpp:1338
+      auto *ReferenceType = dyn_cast<FixedVectorType>(ArgTys[RefArgNumber]);
+      auto *ThisArgVecTy = dyn_cast<FixedVectorType>(Ty);
       if (!ThisArgVecTy || !ReferenceType ||
----------------
efriedma wrote:
> This doesn't look right.
fixed in a previous patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81500