[SVE] Fix wrong usage of getNumElements() in matchIntrinsicType

[david-arm]

Summary

I have changed the ScalableVecArgument case in matchIntrinsicType
to create a new FixedVectorType. This means that the next case we
hit (Vector) will not assert when calling getNumElements(), since
we know that it's always a FixedVectorType. This is a temporary
measure for now, and it will be fixed properly in another patch
that refactors this code.

The changes are covered by this existing test:

CodeGen/AArch64/sve-intrinsics-fp-converts.ll

In addition, I have added a new test to ensure that we correctly
reject SVE intrinsics when called with fixed length vector types.

[efriedma]

llvm/lib/IR/Function.cpp

1184

I don't think we should allow a ScalableVectorType here. Instead, I think we should fix the recursive call to matchIntrinsicType for IITDescriptor::ScalableVecArgument to pass down an appropriate FixedVectorType, and then dyn_cast<FixedVectorType>(Ty) here.

Yes, that's a little weird, but better to make the weirdness self-consistent; my suggestion is consistent with the way we handle ScalableVecArgument in other places.

[efriedma]

Can you add a verifier test that we correctly reject something like the following?

declare <2 x i64> @llvm.aarch64.sve.add.nxv2i64(<2 x i1>, <2 x i64>, <2 x i64>)
define <2 x i64> @f() {
  %r = call <2 x i64> @llvm.aarch64.sve.add.nxv2i64(<2 x i1> zeroinitializer, <2 x i64> zeroinitializer, <2 x i64> zeroinitializer)
  ret <2 x i64> %r
}

[ctetreau]

Is anybody here aware of any plan to deal with IITDescriptor::Vector? Do we need a fixed/scalable split here too?

[ctetreau]

llvm/lib/IR/Function.cpp

1184

Is this a temporary measure until IITDescriptor is fixed? I think "little weird" is a bit of an understatement.

Would it not be better to just do getElementCount().Min here, and make it official that it "works" for scalable vectors and fixed vectors?

1361

NIT: ScalableVectorType actually has a getMinNumElements(). Really, getElementCount() is only really useful if you are dealing with base VectorType objects.

[efriedma]

llvm/lib/IR/Function.cpp

1184

At some point, we should fix IITDescriptor to do something sane, yes. It will be easier to refactor if the existing code doesn't have known bugs.

[david-arm]

Hi @efriedma, I've tried to add a test as you suggested. Although it doesn't exercise the actual code I've changed I added it anyway as I thought maybe it was still worth making sure we reject it.

[ctetreau]

llvm/lib/IR/Function.cpp

1184

It seems to me that it "works" with scalable vectors due to the number of hoops we're jumping through to ensure it actually gets called in the recursive call from ScalableVecArgument. I'd say that doing the explicit conversion from scalable to fixed vector is planting the seed for a bug. If this all needs to get reworked anyways, I think it would be better if this switch arm got changed rather than converting to fixed width vector.

Either way, whichever spot gets changed needs a FIXME comment

[david-arm]

Hi @ctetreau @efriedma, I'm not personally that keen on casting to a FixedVectorType in the Vector case either, but if this is going to be re-written, re-factored in the future then I guess I'm ok with that. What's the best way to proceed here? I have to choose one or the other and if either way is equally bad I'll probably stick with the current patch, but I'm happy to add a FIXME.

[ctetreau]

In D79416#2028914, @david-arm wrote:

Hi @ctetreau @efriedma, I'm not personally that keen on casting to a FixedVectorType in the Vector case either, but if this is going to be re-written, re-factored in the future then I guess I'm ok with that. What's the best way to proceed here? I have to choose one or the other and if either way is equally bad I'll probably stick with the current patch, but I'm happy to add a FIXME.

Even though both options are bad and this is going to be rewritten("we'll fix it later" - famous last words), I think we're early enough in this process where we need to be concerned with setting a good example. Here, one of the variants is called "Vector" and one is called "ScalableVectorArgument". In the Vector branch, it says on the tin that it should match scalable or fixed vectors. In the ScalableVectorArgument branch, it should only match scalable vectors. I think it's less strange to convert a thing that might be a scalable vector to be a fixed vector than it is to convert a thing that must be a scalable vector to a fixed vector. It also is more robust in the case that execution manages to get to the Vector branch with a scalable vector without going through ScalableVectorArgument.

I think what you had in your original patch would be better, with a more explicit TODO about what specifically should be done to fix the situation would be better. If that means that more branches of the recursive call need to be fixed then so be it. The more of the scaffolding of the correct solution that is here, the better.

[efriedma]

llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll

13

This isn't the testcase I asked for; this isn't affected by the patch. Please take a look at my suggested testcase again.

[david-arm]

llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll

13

OK I see. So I wrote a test exactly as you described, but I'm not sure how you want to verify it is rejected? When I run "./bin/opt -verify -S < ../llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll" on your test I get:

define <2 x i64> @add_i64_invalid() {

%out = call <2 x i64> @llvm.aarch64.sve.add.v2i64(<2 x i1> zeroinitializer, <2 x i64> zeroinitializer, <2 x i64> zeroinitializer)
ret <2 x i64> %out

}

Were you expecting to see an error? Or were you simply hoping to see a remangled name for the intrinsic? I'm just not exactly sure what I should be checking.

[efriedma]

llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll

13

Sorry, I messed up the testcase. Should be something like this:

declare <vscale x 4 x i16> @llvm.arm.neon.vcvtfp2hf(<vscale x 4 x float>)
define <vscale x 4 x i16> @f() {
  %r = call <vscale x 4 x i16> @llvm.arm.neon.vcvtfp2hf(<vscale x 4 x float> zeroinitializer)
  ret <vscale x 4 x i16> %r
}

[david-arm]

Hi, I've hopefully added the right tests now to defend the cases I've fixed. I've also added a FIXME to the code that indicates this will be reworked properly soon.

@ctetreau Is a FIXME good enough for now? I can fix this properly in another patch soon afterwards by refactoring the code to avoid the need for IITDescriptor::ScalableVecArgument perhaps? When we create the Vector descriptor I could add extra information that indicates if it is fixed or scalable. Would that be acceptable?

[efriedma]

LGTM

llvm/lib/IR/Function.cpp

switch (D.Kind) {
     case IITDescriptor::Token: return !Ty->isTokenTy();
     case IITDescriptor::Metadata: return !Ty->isMetadataTy();
     case IITDescriptor::Half: return !Ty->isHalfTy();
     case IITDescriptor::Float: return !Ty->isFloatTy();
     case IITDescriptor::Double: return !Ty->isDoubleTy();
     case IITDescriptor::Quad: return !Ty->isFP128Ty();
     case IITDescriptor::Integer: return !Ty->isIntegerTy(D.Integer_Width);
     case IITDescriptor::Vector: {
-      VectorType *VT = dyn_cast<VectorType>(Ty);
+      FixedVectorType *VT = dyn_cast<FixedVectorType>(Ty);
       return !VT || VT->getNumElements() != D.Vector_Width ||
              matchIntrinsicType(VT->getElementType(), Infos, ArgTys,
                                 DeferredChecks, IsDeferredCheck);
     }
     case IITDescriptor::Pointer: {
       PointerType *PT = dyn_cast<PointerType>(Ty);
       return !PT || PT->getAddressSpace() != D.Pointer_AddressSpace ||
              matchIntrinsicType(PT->getElementType(), Infos, ArgTys,
                                 DeferredChecks, IsDeferredCheck);
case IITDescriptor::Subdivide4Argument: {
         NewTy = VectorType::getSubdividedVectorType(VTy, SubDivs);
         return Ty != NewTy;
       }
       return true;
     }
     case IITDescriptor::ScalableVecArgument: {
       if (!isa<ScalableVectorType>(Ty))
         return true;
-      return matchIntrinsicType(Ty, Infos, ArgTys, DeferredChecks,
+      ScalableVectorType *STy = cast<ScalableVectorType>(Ty);
+      unsigned MinElts = STy->getMinNumElements();
+      FixedVectorType *FVTy =
+          FixedVectorType::get(STy->getElementType(), MinElts);
+      return matchIntrinsicType(FVTy, Infos, ArgTys, DeferredChecks,
                                 IsDeferredCheck);
     }
     case IITDescriptor::VecOfBitcastsToInt: {
       if (D.getArgumentNumber() >= ArgTys.size())
         return IsDeferredCheck || DeferCheck(Ty);
       auto *ReferenceType = dyn_cast<VectorType>(ArgTys[D.getArgumentNumber()]);
       auto *ThisArgVecTy = dyn_cast<VectorType>(Ty);
       if (!ThisArgVecTy || !ReferenceType)

llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll

+; RUN: not llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s 2>%t
+; RUN: FileCheck --check-prefix=CHECK-ERROR %s <%t
+
+; CHECK-ERROR: error: invalid forward reference to function 'llvm.aarch64.sve.add.nxv2i64' with wrong type
+
+define <2 x i64> @add_i64_invalid(<2 x i1> %pg, <2 x i64> %a, <2 x i64> %b) {
+  %out = call <2 x i64> @llvm.aarch64.sve.add.nxv2i64(<2 x i1> %pg,
+                                                      <2 x i64> %a,
+                                                      <2 x i64> %b)
+  ret <2 x i64> %out
+}
+
+declare <vscale x  2 x i64> @llvm.aarch64.sve.add.nxv2i64(<vscale x  2 x i1>, <vscale x  2 x i64>, <vscale x  2 x i64>)