[PATCH] D80262: [SVE] Eliminate bad VectorType::getNumElements() calls from ConstantFold

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:574
   // count may be mismatched; don't attempt to handle that here.
+  // FIXME: handle DstTy being a scalable vector
   if ((isa<ConstantVector>(V) || isa<ConstantDataVector>(V)) &&
----------------
The FIXME  is probably unnecessary as it should never be possible to bitcast a fixed-width vector to a scalable vector.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:812
 
+  auto *ValVTy = cast<FixedVectorType>(Val->getType());
+
----------------
How about `extractelement <vscale x 16 x i8> zeroinitializer, i64 0` ?

(This probably cannot be a Constant anymore when the index > 16)


================
Comment at: llvm/lib/IR/ConstantFold.cpp:1004
     }
-  } else if (VectorType *VTy = dyn_cast<VectorType>(C->getType())) {
+  } else if (IsScalableVector) {
     // Do not iterate on scalable vector. The number of elements is unknown at
----------------
This condition/branch has become superfluous given the use of `dyn_cast<FixedVectorType>` below, and the default of `return nullptr`.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:1374
     }
-  } else if (VectorType *VTy = dyn_cast<VectorType>(C1->getType())) {
+  } else if (IsScalableVector) {
     // Do not iterate on scalable vector. The number of elements is unknown at
----------------
Although not as superfluous as the case above, I still think it can be removed.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:2294
+      if (VectorType *VT = dyn_cast<VectorType>(C->getType())) {
+        // FIXME: handle scalable vectors
+        GEPTy = FixedVectorType::get(
----------------
Is it worth just fixing this for scalable vectors instead of adding the FIXME and then doing the wrong thing? It is possible to construct a test-case that breaks this assert.

I think you can use `VectorType::get(..., VT->getElementCount())` here (but it would need a test-case)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80262