[PATCH] D98351: [llvm-opt] Bug fix within combining FP vectors

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:1581
+
   for (unsigned i = 0; i != NumElts; ++i) {
     auto *CFP = dyn_cast_or_null<ConstantFP>(CV->getAggregateElement(i));
----------------
This is not correct, because we can't iterate the number of elements in a vector of which we don't know how many elements there are.

The choice here is to either:
* Check if `V` is a splatvector of some FP constant, and then try to shrink that FP constant's Type to something smaller. This is only useful if the code that calls this function can actually do something with that information for scalable vectors.
* Return `nullptr` if the type is a ScalableVectorType, possibly with a FIXME added.



================
Comment at: llvm/test/Transforms/InstCombine/instcombine-vectors.ll:1
+; RUN: opt -instcombine -march=armv8.1-a+sve -S -o - < %s | FileCheck %s
+
----------------
nit: SVE is a feature of Armv8.2.

Regardless, the specific architecture version isn't that relevant here. What is important is that it knows its target is aarch64 (e.g. ` -mtriple=aarch64-none-linux-gnu`)  and that the SVE attribute is enabled (`-mattr=+sve`).


================
Comment at: llvm/test/Transforms/InstCombine/instcombine-vectors.ll:4
+define <vscale x 2 x float> @foo(<vscale x 2 x double> %a) {
+  ; CHECK-LABEL: @foo
+  %1 = fadd <vscale x 2 x double> %a, shufflevector (<vscale x 2 x double> insertelement (<vscale x 2 x double> poison, double -1.000000e+00, i32 0), <vscale x 2 x double> poison, <vscale x 2 x i32> zeroinitializer)
----------------
Just checking the label is not sufficient. It is good practice to check the output and make sure the output from the RUN line is sensible. In this case, I would expect InstCombine to optimize the operation in some way, but I can't see if that has happened without the added CHECK lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98351