[PATCH] D81504: [SVE] Remove calls to VectorType::getNumElements from Analysis

[Christopher Tetreault via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:111
 
   if (auto *VTy = dyn_cast<VectorType>(C->getType())) {
     // Handle a vector->scalar integer/fp cast.
----------------
RKSimon wrote:
> Just use FixedVectorType in the dyn_cast here?
That would be a behavior change. The idea for this patch is to remove the invalid calls to VectorType::getNumElements with no other behavior changes. The original code enters this branch for scalable vectors, then possibly miscompiles or crashes further down the line. Now it enters this branch for scalable vectors and crashes immediately. As follow up work, cases like this should get looked at.

We're trying to avoid behavior changes of the form "this branch used to be taken for scalable vectors, now it isn't" because 1) There are a ton of such cases, and if I had to write tests for all of them right now this work would never get done and 2) A lot of these cases are actually impossible to trigger for scalable vectors (as of today), so we can't actually write tests for them. I haven't analyzed this particular one, but in general anything that has to do with constants is hard to test with scalable vectors because there are only a few scalable vectors that it's actually possible to construct constants for today.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:141
   // The code below only handles casts to vectors currently.
   auto *DestVTy = dyn_cast<VectorType>(DestTy);
   if (!DestVTy)
----------------
RKSimon wrote:
> dyn_cast<FixedVectorType> ?
this would be a behavior change


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:950
   if (Op1C && VTy) {
-    unsigned NumElts = VTy->getNumElements();
+    unsigned NumElts = cast<FixedVectorType>(VTy)->getNumElements();
     for (unsigned i = 0; i != NumElts; ++i) {
----------------
RKSimon wrote:
> VTy is already FixedVectorType?
I believe that this is a rebase artifact. I'll fix it.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:815
   VectorType *VecTy1 = dyn_cast<VectorType>(V1->getType());
   VectorType *VecTy2 = dyn_cast<VectorType>(V2->getType());
   assert(VecTy1 && VecTy2 &&
----------------
RKSimon wrote:
> dyn_cast<FixedVectorType>
this would be a behavior change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81504