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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:812
 
+  auto *ValVTy = cast<FixedVectorType>(Val->getType());
+
----------------
sdesmalen wrote:
> How about `extractelement <vscale x 16 x i8> zeroinitializer, i64 0` ?
> 
> (This probably cannot be a Constant anymore when the index > 16)
I don't quite remember what (if anything) we decided to do about this case last thursday. I believe I was on the "it might be out of bounds, but we can't know so we shouldn't do anything" side of the argument, but I don't remember which side won.

Regardless, it would probably be more appropriate to just skip this particular transformation rather than letting it explode. I think it makes sense to just do that for now and revisit when we have all the correctness issues sorted out. I suppose this represents a behavior change, and needs a test...


================
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
----------------
sdesmalen wrote:
> This condition/branch has become superfluous given the use of `dyn_cast<FixedVectorType>` below, and the default of `return nullptr`.
good catch


================
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
----------------
sdesmalen wrote:
> Although not as superfluous as the case above, I still think it can be removed.
This could potentially result in a behavior change, which I am trying to avoid as much as possible in this patch. If you're sure it doesn't, I can change it. Otherwise, I can add a FIXME.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:2294
+      if (VectorType *VT = dyn_cast<VectorType>(C->getType())) {
+        // FIXME: handle scalable vectors
+        GEPTy = FixedVectorType::get(
----------------
sdesmalen wrote:
> 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)
> 
> 
If I had the authority to make the call, I'd say we should just replace all instances of `VectorType::get(..., SomeVec->getNumElements() /*, false */)` with `VectorType::get(..., SomeVec->getElementCount())` without tests, as the original form is almost certainly a bug.

Unfortunately, others disagree with this.  While it pains me to write this code, my current goal is to just get rid of the bad usages of VectorType::getNumElements in as frictionless a manner as possible, which means taking the coward's way out so as not to get bogged down writing a billion tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80262