[SVE] Remove calls to VectorType::getNumElements from AggressiveInstCombine

[ctetreau]

The goal for this patch is to remove all invalid calls to VectorType::getNumElements with as few behavior changes as possible. If some code previously called getNumElements via a base VectorType pointer, we are assuming that it meant to treat the vector as a fixed width vector, and are instead unconditionally casting to FixedVectorType. Any test failures that arise from this change are cases where the assumption wasn't correct, and these are being handled on a case by case basis.

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 we 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. 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. This means that some very strange code will result. Things like:

if (auto *VTy = dyn_cast<VectorType>(Ty)) {
   unsinged NumElts = cast<FixedVectorType>(VTy);

If we just dyn_cast to FixedVectorType, then a branch that used to be taken for scalable vectors will now be skipped, and this is a behavior change that requires a new test. It is my hope that this code will look very strange to people familiar with the code in question, and that it will get fixed over time. If this does crash, it will be very obvious that the cast probably isn't correct. People are unlikely to see this and ask themselves "is VTy actually supposed to be a FixedVectorType? Did *I* break this?" We are also intentionally avoiding some "obvious" fixes like auto *VTy = VectorType::get(SomeTy, SomeVTy->getNumElements()) => auto *VTy = VectorType::get(SomeTy, SomeVTy->getElementCount()). While it seems obvious that the second form should be fine, it is technically a behavior change that requires a new test, and this sort of thing happens a lot. As with the dyn_cast issue, if we had to write tests for every instance, this work would never get done.

While reviewing this code, please try to keep this in mind.

[fpetrogalli]

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp

294–295

Hi @ctetreau ,

why casting into FixedVectorType when FixedVectorType::get is already enforcing fixed vector types? I guess enforcing fixed vectors once is enough.

My preference would be to use: return FixedVectorType::get(Ty, VTy->getElementCount()); If runtime errors have to be raised, I'd rather have it raised in a "standard" place, by invoking FixedVectorType::get on a scalable ElementCount, rather than by an explicit cast that will have no message associated.

Francesco

[ctetreau]

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp

294–295

@fpetrogalli Here we're casting VTy to FixedVectorType in order to call getNumElements().

Currently, if you call FixedVectorType::get(Ty, VTy->getElementCount());, it will just call VectorType::get(Type *, ElementCount), which can actually return a scalable vector. I should probably fix this if for no other reason than it seems odd that you can do this. I guess idiomatic LLVM thing to do is to have FixedVectorType::get(SomeTy, {SomeInt, true}) should return nullptr? I'll open a separate patch for that and put you on as a reviewer

That said, anything short of a VectorType::get(Type *, ElementCount EC) that asserts if EC.Scalable is true would result in a behavior change here, which we're trying to avoid. If get can return nullptr, then this function can now return nullptr. If get can return a scalable vector, then this function can now too (it never did before). I would love to fix all of these scalable -> fixed vector demotions by calling getElementCount with no tests because "if VTy is scalable, then FIxedVectorType::get(Ty, VTy->getNumElements()) is almost certainly a bug" but the policy is any behavior change needs a test and exchanging one bug for (potentially) another is a behavior change.

[ctetreau]

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp

294–295

I responded to your comment in D82211. I agree that these are all morally behavior changes. However, the assumption is that any call to getNumElements that, when cast to FixedVectorType, does not cause a test failure is always a FixedVectorType.

[ctetreau]

rebase

[fpetrogalli]

Hi @ctetreau ,

thank you for explaining. Sorry it took so long to realize I needed to come back to you on this.

LGTM!

Francesco

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp

 
 /// Given a reduced scalar type \p Ty and a \p V value, return a reduced type
 /// for \p V, according to its type, if it vector type, return the vector
 /// version of \p Ty, otherwise return \p Ty.
 static Type *getReducedType(Value *V, Type *Ty) {
   assert(Ty && !Ty->isVectorTy() && "Expect Scalar Type");
   if (auto *VTy = dyn_cast<VectorType>(V->getType())) {
     // FIXME: should this handle scalable vectors?
-    return FixedVectorType::get(Ty, VTy->getNumElements());
+    return FixedVectorType::get(Ty,
+                                cast<FixedVectorType>(VTy)->getNumElements());
   }
   return Ty;
 }
 
 Value *TruncInstCombine::getReducedOperand(Value *V, Type *SclTy) {
   Type *Ty = getReducedType(V, SclTy);
   if (auto *C = dyn_cast<Constant>(V)) {
     C = ConstantExpr::getIntegerCast(C, Ty, false);