This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by david-arm on May 5 2020, 6:14 AM.

Details

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.

Diff Detail

Event Timeline

david-arm created this revision.May 5 2020, 6:14 AM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
efriedma added inline comments.May 5 2020, 1:36 PM
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.

david-arm updated this revision to Diff 262310.May 6 2020, 1:10 AM
david-arm edited the summary of this revision. (Show Details)
david-arm marked an inline comment as done.

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
}

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

ctetreau added inline comments.May 6 2020, 2:58 PM
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?

1363

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

efriedma added inline comments.May 6 2020, 5:55 PM
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 updated this revision to Diff 262589.May 7 2020, 3:33 AM
david-arm edited the summary of this revision. (Show Details)
david-arm marked an inline comment as done.

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 added inline comments.May 7 2020, 8:46 AM
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

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.

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 accepted this revision.May 11 2020, 10:23 AM
efriedma added inline comments.
llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll
14

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

This revision is now accepted and ready to land.May 11 2020, 10:23 AM
david-arm marked an inline comment as done.May 12 2020, 6:12 AM
david-arm added inline comments.
llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll
14

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 added inline comments.May 12 2020, 12:43 PM
llvm/test/CodeGen/AArch64/sve-bad-intrinsics.ll
14

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 edited the summary of this revision. (Show Details)

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?

This revision was automatically updated to reflect the committed changes.