[InstCombine] shrinkFPConstantVector(): scalable vector support

[nasherm]

Summary

A bug was found within InstCombineCasts where a function call
is only implemented to work with FixedVectors. This caused a
crash when a ScalableVector was passed to this function.
This commit introduces a regression test which recreates the
failure and a bug fix.

[sdesmalen]

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1582

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.

llvm/test/Transforms/InstCombine/instcombine-vectors.ll

1 ↗

(On Diff #329660)

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).

4 ↗

(On Diff #329660)

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.

[CarolineConcatto]

Hey Nash,

I also added some comments.
I hope it is ok.

Carol

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1598

Just in case, I don't think that you should cast this to be ScalableVectorType.
I believe you can leave the return as it is originally
Especially if Sander is saying to have an earlier return when doing the for.

llvm/test/Transforms/InstCombine/instcombine-vectors.ll

2 ↗

(On Diff #329660)

Usually we also add this line in our tests:
; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t

; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.$
; WARN-NOT: warning

And I believe this test should be in inside test/Transforms/InstCombine/AArch64

[nasherm]

Response to Carol and Sander's comments. I have gone for an
approach which implements an optimisation for the case
where we have a trunc op of the following form

fptrunc (OpI (fpextend v) (fpextend u))

and in the case where one or both of u and v
are scalable splat vectors, we truncate to the lowest
common FP width which maintains precision. I have also
implemented a test which showcases this optimisation.

[david-arm]

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1602

Hi @nasherm, is it possible to just add code to this function that does something like this?

if (auto *CV = dyn_cast<Constant>(V))
  if (auto *FPExt = dyn_cast<FPExtInst>(CV->getOperand(0)))
    return FPExt->getOperand(0)->getType();

Then in shrinkFPConstantVector you could just bail out early on for scalable vectors? Does that give you the same effect?

[nasherm]

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1602

That doesn't work as the the second dyn_cast will fail. It seems like FPExt within a Constant can't be cast in that way.

[sdesmalen]

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1605

I think the reason it gets to shrinkFPConstant is because the above dyn_cast<FPExtInst> returns false, even though V is a fpext constant expression.

If you add:

+  if (auto *FPCExt = dyn_cast<ConstantExpr>(V))
+    if (FPCExt->getOpcode() == Instruction::FPExt)
+      return FPCExt->getOperand(0)->getType();

Then your test will pass.

The problem is quite specific to scalable vector splat-operations which cannot constant fold into something simpler, thus leading to a ConstantExpression. I'm not entirely sure if we should duplicate such code for ConstantExprs. In this case it seems like a pretty trivial fix that isn't that much more complicated than bailing out for scalable vectors, so perhaps this is fine, but it does seem a bit arbitrary.

Personally I would be happy to go either way, either the proposed solution or bailing out and avoiding this improvement until we hit this as something that needs fixing.

[nasherm]

In response to Sander and David's approach, I have
refactored the solution to use the simpler check
for splat vectors i.e. checking with constant
fpext expressions

[sdesmalen]

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1572

If you change VectorType to FixedVectorType here, it will automatically bail out on line 1573, and then you can undo the remaining changes in this function.

[CarolineConcatto]

Just in case ...

llvm/test/Transforms/InstCombine/AArch64/sve-splat.ll

9 ↗

(On Diff #331596)

; CHECK-NEXT: %1 = fadd -> ; CHECK-NEXT: %[[FADD:.*]] = fadd ....
or something like this.

10 ↗

(On Diff #331596)

ret <vscale x 2 x float> %1 -> ret <vscale x 2 x float> %[[FADD]]

[nasherm]

Addressed Sander's comment on casting to FixedVectorType as well
as Carol's suggestion for regression test.

[david-arm]

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.

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

1580

nit: I think it's probably better to use the same terminology as used elsewhere in this function, i.e. minimal type?

1614

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.

[lebedev.ri]

llvm/test/Transforms/InstCombine/AArch64/sve-splat.ll

1 ↗

(On Diff #331880)

Do you really need anything other than plain -instcombine here?

[nasherm]

Response to David's comments

[nasherm]

llvm/test/Transforms/InstCombine/AArch64/sve-splat.ll

1 ↗

(On Diff #331880)

No, in practice there's no need. I added the triple and attributes to be in-line with the rest of the AArch64 tests.

[lebedev.ri]

llvm/test/Transforms/InstCombine/AArch64/sve-splat.ll

1 ↗

(On Diff #331880)

This shouldn't even be a target-specific test, it doesn't test anything aarch64-specific.

[nasherm]

llvm/test/Transforms/InstCombine/AArch64/sve-splat.ll

1 ↗

(On Diff #331880)

I'm writing a (very) small new patch which will generalise this test per your previous comment. I'll add you to the subscribers/reviewers list once it's on phabricator. :)

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

static Type *shrinkFPConstant(ConstantFP *CFP) {
   return nullptr;
 }
 
 // Determine if this is a vector of ConstantFPs and if so, return the minimal
 // type we can safely truncate all elements to.
 // TODO: Make these support undef elements.
 static Type *shrinkFPConstantVector(Value *V) {
   auto *CV = dyn_cast<Constant>(V);
-  auto *CVVTy = dyn_cast<VectorType>(V->getType());
+  auto *CVVTy = dyn_cast<FixedVectorType>(V->getType());
   if (!CV || !CVVTy)
     return nullptr;
 
   Type *MinType = nullptr;
 
-  unsigned NumElts = cast<FixedVectorType>(CVVTy)->getNumElements();
+  unsigned NumElts = CVVTy->getNumElements();
+
+  // For fixed-width vectors we find the minimal type by looking
+  // through the constant values of the vector.
   for (unsigned i = 0; i != NumElts; ++i) {
     auto *CFP = dyn_cast_or_null<ConstantFP>(CV->getAggregateElement(i));
     if (!CFP)
       return nullptr;
 
     Type *T = shrinkFPConstant(CFP);
     if (!T)
       return nullptr;
 
     // If we haven't found a type yet or this type has a larger mantissa than
     // our previous type, this is our new minimal type.
     if (!MinType || T->getFPMantissaWidth() > MinType->getFPMantissaWidth())
       MinType = T;
   }
 
   // Make a vector type from the minimal type.
   return FixedVectorType::get(MinType, NumElts);
 }
 
 /// Find the minimum FP type we can safely truncate to.
 static Type *getMinimumFPType(Value *V) {
   if (auto *FPExt = dyn_cast<FPExtInst>(V))
     return FPExt->getOperand(0)->getType();
 
   // If this value is a constant, return the constant in the smallest FP type
   // that can accurately represent it.  This allows us to turn
   // (float)((double)X+2.0) into x+2.0f.
   if (auto *CFP = dyn_cast<ConstantFP>(V))
     if (Type *T = shrinkFPConstant(CFP))
       return T;
 
-  // Try to shrink a vector of FP constants.
+  // 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.
+  if (auto *FPCExt = dyn_cast<ConstantExpr>(V))
+    if (FPCExt->getOpcode() == Instruction::FPExt)
+      return FPCExt->getOperand(0)->getType();
+
+  // Try to shrink a vector of FP constants. This returns nullptr on scalable
+  // vectors
   if (Type *T = shrinkFPConstantVector(V))
     return T;
 
   return V->getType();
 }
 
 /// Return true if the cast from integer to FP can be proven to be exact for all
 /// possible inputs (the conversion does not lose any precision).

llvm/test/Transforms/InstCombine/AArch64/sve-const-fp-splat.ll

+; RUN: opt -instcombine -mtriple aarch64-linux-gnu -mattr=+sve -S -o - < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning
+
+define <vscale x 2 x float> @shrink_splat_scalable_extend(<vscale x 2 x float> %a) {
+  ; CHECK-LABEL: @shrink_splat_scalable_extend
+  ; CHECK-NEXT:  %[[FADD:.*]] = fadd <vscale x 2 x float> %a, shufflevector (<vscale x 2 x float> insertelement (<vscale x 2 x float> undef, float -1.000000e+00, i32 0), <vscale x 2 x float> undef, <vscale x 2 x i32> zeroinitializer)
+  ; CHECK-NEXT:  ret <vscale x 2 x float> %[[FADD]]
+  %1 = shufflevector <vscale x 2 x float> insertelement (<vscale x 2 x float> undef, float -1.000000e+00, i32 0), <vscale x 2 x float> undef, <vscale x 2 x i32> zeroinitializer
+  %2 = fpext <vscale x 2 x float> %a to <vscale x 2 x double>
+  %3 = fpext <vscale x 2 x float> %1 to <vscale x 2 x double>
+  %4 = fadd <vscale x 2 x double> %2, %3
+  %5 = fptrunc <vscale x 2 x double> %4 to <vscale x 2 x float>
+  ret <vscale x 2 x float> %5
+}