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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked an inline comment as done.
ctetreau added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp:294
     // FIXME: should this handle scalable vectors?
-    return FixedVectorType::get(Ty, VTy->getNumElements());
+    return FixedVectorType::get(Ty,
+                                cast<FixedVectorType>(VTy)->getNumElements());
----------------
fpetrogalli wrote:
> 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
> 
> 
@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82218