Page MenuHomePhabricator

[IntrinsicEmitter] Support scalable vectors in intrinsics
ClosedPublic

Authored by c-rhodes on Aug 8 2019, 2:27 AM.

Details

Summary

This patch adds support for scalable vectors in intrinsics, enabling
intrinsics such as the following to be defined:

declare <vscale x 4 x i32> @llvm.something.nxv4i32(<vscale x 4 x i32>)

Support for this is implemented by defining a new type descriptor for
scalable vectors and adding mangling support for scalable vector types
in the name mangling scheme used by 'any' types in intrinsic signatures.

Tests have been added for IRBuilder to test scalable vectors work as
expected when using intrinsics through this interface. This required
implementing an intrinsic that is explicitly defined with scalable
vectors, e.g. LLVMType<nxv4i32>, an SVE floating-point convert
intrinsic was used for this. The behaviour of the overloaded type
LLVMScalarOrSameVectorWidth with scalable vectors is tested using the
existing masked load intrinsic. Also added an .ll test to test the
Verifier catches a bad intrinsic argument when passing a fixed-width
predicate (mask) to the masked.load intrinsic where a scalable is
expected.

Patch by Paul Walker

Diff Detail

Event Timeline

c-rhodes created this revision.Aug 8 2019, 2:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 2:27 AM

I've posted a follow-up patch (D65931) to Phabricator that tests this functionality by implementing intrinsics for a couple of SVE arithmetic instructions.

Thanks for this patch @c-rhodes!

I see the other patch adds the intrinsics that use (and test) this functionality, but I think it would help to also have some tests in this patch. Specifically to assert that passing a fixed-width vector where a scalable vector is expected (and vice-versa), fails.
You can probably tests these by just running opt -verify.

Specifically to assert that passing a fixed-width vector where a scalable vector is expected (and vice-versa), fails.

I'm not sure such a test provides much value, the test will fail but I don't think it would test anything in the context of this patch, I'm not sure exactly where the error will come from but maybe the IR Verifier?

If we want to test the type constraints for scalable/non-scalable vectors maybe we could add an any type for scalable vectors e.g. any_scalable_vector_type, this could give us tighter type constraints for intrinsics that only work with scalable vectors and hopefully something to test.

I'd be happy to move the intrinsic definitions from the follow-on patch to at least have a test here.

If we want to test the type constraints for scalable/non-scalable vectors maybe we could add an any type for scalable vectors e.g. any_scalable_vector_type, this could give us tighter type constraints for intrinsics that only work with scalable vectors and hopefully something to test.

I like this idea. I agree with Sander that we want tests with this patch.

c-rhodes updated this revision to Diff 215550.Aug 16 2019, 3:40 AM
c-rhodes edited the summary of this revision. (Show Details)
  • Moved the intrinsic implementation for abs/neg (without codegen) from D65931 so this patch can be tested.
  • Added tests for IRBuilder to test scalable vectors work as expected when using intrinsics through the IRBuilder interface. This required implementing an intrinsic that is explicitly defined with scalable vectors, e.g. LLVMType<nxv4i32>, an SVE floating-point convert intrinsic was used for this. Also tested the behaviour of the overloaded type LLVMScalarOrSameVectorWidth with scalable vectors using the existing masked load intrinsic.
  • Replace uses of getNumElements with getElementCount when handling intrinsic type LLVMScalarOrSameVectorWidth so it can correctly support scalable vectors.
  • Added an .ll test to test the Verifier catches a bad intrinsic argument when passing a fixed-width predicate (mask) to the abs intrinsic where a scalable is expected.
sdesmalen added inline comments.Aug 21 2019, 7:18 AM
test/Verifier/intrinsic-bad-arg-type.ll
7 ↗(On Diff #215550)

Can you also test this with llvm.masked.load ? If so, then you can leave abs/neg for D65931 and simplify this one.

c-rhodes marked 3 inline comments as done.Aug 22 2019, 3:50 AM
c-rhodes added inline comments.
test/Verifier/intrinsic-bad-arg-type.ll
7 ↗(On Diff #215550)

I've tried the following IR based on your suggestion but hit an assert with a debug build:

define <vscale x 4 x i32> @masked_load(<vscale x 4 x i32>* %addr, <4 x i1> %mask, <vscale x 4 x i32> %dst) {
  %res = call <vscale x 4 x i32> @llvm.masked.load.nxv4i32(<vscale x 4 x i32>* %addr, i32 4, <4 x i1> %mask, <vscale x 4 x i32> %dst)
  ret <vscale x 4 x i32> %res
}
declare <vscale x 4 x i32> @llvm.masked.load.nxv4i32(<vscale x 4 x i32>*, i32, <4 x i1>, <vscale x 4 x i32>)

Opt output:

opt -S -verify masked-load-scalable-mask.ll

opt: ./llvm/lib/IR/Instructions.cpp:405: void llvm::CallInst::init(llvm::FunctionType*, llvm::Value*, llvm::ArrayRef<llvm::Value*>, llvm::ArrayRef<llvm::OperandBundleDefT<llvm::Value*> >, const llvm::Twine&): Assertion `(i >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType()) && "Calling a function with a bad signature!"' failed.

For the abs intrinsic test the same Verifier error is hit regardless of build type, the intrinsic definitions are similar with respect to the mask (LLVMScalarOrSameVectorWidth<0, llvm_i1_ty>) so I'm not exactly sure what's going on here.

c-rhodes updated this revision to Diff 216610.Aug 22 2019, 7:29 AM
c-rhodes edited the summary of this revision. (Show Details)
  • Used existing masked.load intrinsic in .ll test that checks the verifier catches bad type when passing a fixed-width vector where a scalable is expected.
  • Removed definition of abs and neg intrinsics that were previously used in the above test, these are implemented in the follow-up patch.
  • Updated call to CreateIntrinsic in IRBuilderTest to make types argument empty for call to fp-convert intrinsic, only required for overloaded types.
c-rhodes marked an inline comment as done.Aug 22 2019, 7:58 AM
c-rhodes added inline comments.
test/Verifier/intrinsic-bad-arg-type.ll
7 ↗(On Diff #215550)

I fixed the issue I hit by specifying the intrinsic as @llvm.masked.load.nxv4i32.p0nxv4i32, wasn't aware the last bit was necessary but it brings the error handling inline with what I was seeing for the abs test. I've updated the patch.

sdesmalen accepted this revision.Aug 23 2019, 4:00 AM

LGTM.

test/Verifier/intrinsic-bad-arg-type.ll
7 ↗(On Diff #215550)

Yes, that happens because when you don't specify the full types in the mangled name, UpgradeCallsToIntrinsic will upgrade the intrinsic and the call. This will call IRBuidler, which for Debug builds asserts that the types match, and will now fail because <4 x i1> is not the expected <vscale x 4 x 1>. If you specify the mangled intrinsic name correctly, the call is not upgraded and the code in matchIntrinsicType is exercised, thus giving a diagnostic that Intrinsic has incorrect argument type!.

This revision is now accepted and ready to land.Aug 23 2019, 4:00 AM
This revision was automatically updated to reflect the committed changes.