[PATCH] D62995: [IntrinsicEmitter] Extend argument overloading with forward references.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked an inline comment as done.
sdesmalen added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.td:1185-1202
+def int_experimental_dummy_backward : Intrinsic<[llvm_anyvector_ty],
+                                                [llvm_anyvector_ty,
+                                                 LLVMVectorOfAnyPointersToElt<0>,
+                                                 LLVMVectorOfAnyPointersToElt<1>,
+                                                 LLVMVectorElementType<0>], []>;
+
+def int_experimental_dummy_forward : Intrinsic<[LLVMVectorOfAnyPointersToElt<0>],
----------------
arsenm wrote:
> sdesmalen wrote:
> > arsenm wrote:
> > > These should just go to a TableGen test file instead of defining a real intrinsic
> > 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.
> Can you just add some cases with the real intrinsics after the use patch is committed?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62995/new/

https://reviews.llvm.org/D62995