This is an archive of the discontinued LLVM Phabricator instance.

[IntrinsicEmitter] Extend argument overloading with forward references.
ClosedPublic

Authored by sdesmalen on Jun 7 2019, 1:05 AM.

Details

Summary

Extend the mechanism to overload intrinsic arguments by using either
backward or forward references to the overloadable arguments.

In for example:

def int_something : Intrinsic<[LLVMPointerToElt<0>],
                              [llvm_anyvector_ty], []>;

LLVMPointerToElt<0> is a forward reference to the overloadable operand
of type 'llvm_anyvector_ty' and would allow intrinsics such as:

declare i32* @llvm.something.v4i32(<4 x i32>);
declare i64* @llvm.something.v2i64(<2 x i64>);

where the result pointer type is deduced from the element type of the
first argument.

If the returned pointer is not a pointer to the element type, LLVM will
give an error:

Intrinsic has incorrect return type!
i64* (<4 x i32>)* @llvm.something.v4i32

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Jun 7 2019, 1:05 AM
arsenm added inline comments.Jun 7 2019, 6:07 AM
include/llvm/IR/Intrinsics.td
1185–1202 ↗(On Diff #203502)

These should just go to a TableGen test file instead of defining a real intrinsic

lib/IR/Function.cpp
1195 ↗(On Diff #203502)

IsDeferredCheck || DeferCheck(Ty)

sdesmalen marked an inline comment as done.Jun 7 2019, 7:00 AM
sdesmalen added inline comments.
include/llvm/IR/Intrinsics.td
1185–1202 ↗(On Diff #203502)

I agree these shouldn't be here, but I don't really know any other way to test the changes to matchIntrinsicType in lib/IR/Function.cpp. If we move these these to a separate TableGen test, it will only test the changes to TableGen.

nikic added a subscriber: nikic.Jun 8 2019, 8:32 AM
dmgreen added a subscriber: dmgreen.Jun 9 2019, 1:09 PM
arsenm added inline comments.Jun 10 2019, 2:53 PM
include/llvm/IR/Intrinsics.td
1185–1202 ↗(On Diff #203502)

Can you just add some cases with the real intrinsics after the use patch is committed?

sdesmalen marked an inline comment as done.Jun 11 2019, 9:22 AM
sdesmalen added inline comments.
include/llvm/IR/Intrinsics.td
1185–1202 ↗(On Diff #203502)

Sure, no problem!

From these tests int_experimental_dummy_forward_struct_ret can be tested by changing AdvSIMD_3Vec_Load_Intrinsic in IntrinsicsAArch64.td:

class AdvSIMD_3Vec_Load_Intrinsic
  : Intrinsic<[llvm_anyvector_ty, LLVMMatchType<0>, LLVMMatchType<0>],
              [LLVMAnyPointerType<LLVMMatchType<0>>],
              [IntrReadMem, IntrArgMemOnly]>;

to

class AdvSIMD_3Vec_Load_Intrinsic
  : Intrinsic<[LLVMMatchType<0>, LLVMMatchType<0>, llvm_anyvector_ty],
              [LLVMAnyPointerType<LLVMMatchType<0>>],
              [IntrReadMem, IntrArgMemOnly]>;

The _forward_ test that uses LLVMVectorOfAnyPointersToElt is a bit trickier. The masked.gather is the only intrinsic that uses this LLVMMatchTy, but if we change:

def int_masked_gather: Intrinsic<[llvm_anyvector_ty],
                                 [LLVMVectorOfAnyPointersToElt<0>, llvm_i32_ty,
                                  LLVMScalarOrSameVectorWidth<0, llvm_i1_ty>,
                                  LLVMMatchType<0>],
                                 [IntrReadMem, ImmArg<1>]>;

into:

def int_masked_gather: Intrinsic<[LLVMMatchType<0>],
                                 [LLVMVectorOfAnyPointersToElt<0>, llvm_i32_ty,
                                  LLVMScalarOrSameVectorWidth<0, llvm_i1_ty>,
                                  llvm_anyvector_ty],
                                  [IntrReadMem, ImmArg<1>]>;

Then:

declare <4 x float> @llvm.masked.gather.v4f32.v4p0f32(<4 x float*>, i32 immarg, <4 x i1>, <4 x float>)

Would change into:

declare <4 x float> @llvm.masked.gather.v4p0f32.v4f32(<4 x float*>, i32 immarg, <4 x i1>, <4 x float>)

And IRBuilder would need to be updated to change the type-list of the function, with the pointer type <4 x float*> before the vector type <4 x float>. I guess this is something we don't want to change? (probably also because we don't want to overload based on the passthu value).

For SVE ACLE intrinsics we rely more heavily on these forward references and we'll be adding more intrinsics that test this behaviour in the near future. (note that I've tested this implementation downstream with our SVE ACLE intrinsics).

In any case I'll remove the "dummy" tests added in this revision, and create a new patch that changes AdvSIMD_3Vec_Load_Intrinsic along with a corresponding test similar to the one in test/Verifier/intrinsic-arg-overloading-struct-ret.ll.

sdesmalen updated this revision to Diff 204237.EditedJun 12 2019, 2:17 AM
  • Changed: return IsDeferredCheck ? true : DeferCheck(Ty); => return IsDeferredCheck || DeferCheck(Ty);
  • Use DeferredChecks.size() to determine what MatchIntrinsicTypesResult to return, instead of incorrectly relying on DeferredChecks.end() iterator.
  • Removed 'dummy' intrinsics. I will post a separate patch that tests the behaviour tested with llvm.experimental.dummy.forward.struct.ret on existing intrinsics.
sdesmalen marked an inline comment as done.Jun 12 2019, 2:18 AM
arsenm accepted this revision.Jun 12 2019, 7:19 AM

LGTM with nit

utils/TableGen/IntrinsicEmitter.cpp
386 ↗(On Diff #204237)

Extra space before (

This revision is now accepted and ready to land.Jun 12 2019, 7:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 1:18 AM
sdesmalen marked an inline comment as done.Jun 13 2019, 1:18 AM

LGTM with nit

Thanks for the review!