[PATCH] D98351: [InstCombine] shrinkFPConstantVector(): scalable vector support

[David Sherwood via Phabricator via llvm-commits]

david-arm accepted this revision.
david-arm added a comment.
This revision is now accepted and ready to land.

LGTM! Hi @nasherm, thanks for addressing all the review comments! I just have a few minor nits.

Perhaps it's worth renaming the test file from `sve-splat.ll` to something like 'sve-const-fp-splat.ll'? The reason being that `sve-splat.ll` is quite generic and could apply to many operations in different situations, including splats of non-constant values.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:1580
+
+  // For fixed-width vectors we find the min-type by looking
+  // through the constant values of the vector.
----------------
nit: I think it's probably better to use the same terminology as used elsewhere in this function, i.e. `minimal type`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:1614
+  // We only can correctly find a MinType for a ScalableVector if the vector
+  // is a splat-vector, otherwise we can't shrink due to the runtime-defined
+  // vscale value. Vectors of this type will be wrapped in a fpext constant.
----------------
nit: I think it's probably sufficient just to say something like:

```
  // We can only correctly find a minimum type for a scalable vector when it is a splat.
  // For splats of constant values the fpext is wrapped up as a ConstantExpr.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98351