Page MenuHomePhabricator

Support arbitrary addrspace pointers in masked load/store intrinsics
ClosedPublic

Authored by apilipenko on Feb 15 2016, 8:24 AM.

Details

Summary

Masked load/store are overloaded intrinsics, the only generic type is the type of the value being loaded/stored. The signature of the intrinsic is generated based on this type. The type of the pointer argument is generated as a pointer to the return type with default addrspace. E.g.:

declare <8 x i32> @llvm.masked.load.v8i32(<8 x i32>*, i32, <8 x i1>, <8 x i32>)

The problem occurs when loop-vectorize tries to use @llvm.masked.load/store intrinsic for a non-default addrspace pointer. It fails with "Calling a function with a bad signature!" assertion in CallInst constructor because it tries to pass a non-default addrspace pointer to the pointer argument which has default addrspace.

The proposed fix is to add pointer type as another overloaded type to @llvm.masked.load/store intrinsics. So, there will be something like for an addrspace(1) pointer:

declare <8 x i32> @llvm.masked.load.v8i32.p1v8i32(<8 x i32> addrspace(1)*, i32, <8 x i1>, <8 x i32>)

This patch updates:

  • Intrinsics description in Intrinsics.td
  • LangRef.rst
  • IRBuilder
  • Existing testcases
  • Adds a test case to masked_load_store.ll which check that a loop with non-default addrspace pointers is correctly vectorized

The problem is relevant for gather/scatter intrinsics as well. This patch doesn't address them because loop-vectorize doesn't emit them yet.

Diff Detail

Repository
rL LLVM

Event Timeline

apilipenko updated this revision to Diff 47994.Feb 15 2016, 8:24 AM
apilipenko retitled this revision from to Support arbitrary addrspace pointers in masked load/store intrinsics.
apilipenko updated this object.
apilipenko added a reviewer: delena.
apilipenko added a subscriber: llvm-commits.
delena edited edge metadata.Feb 15 2016, 10:10 AM

I think it really complicates the form. I don't like this change. I propose you to open discussion in LLVM-dev forum.

arsenm added a subscriber: arsenm.Feb 15 2016, 11:38 AM
arsenm added inline comments.
docs/LangRef.rst
11341 ↗(On Diff #47994)

Typo herei n the name mangling

pjcoup added a subscriber: pjcoup.Feb 16 2016, 8:47 AM
reames added a subscriber: reames.Feb 16 2016, 2:47 PM

Minor comments on the patch inline.

Elena - This seems like a necessary enhancement to support non-zero address spaces - which is a documented and supported part of the IR. I understand your concern about readability, but I think that's outweighed by correctness. Do you have any suggestions on how to improve this patch while meeting it's objective?

docs/LangRef.rst
11336 ↗(On Diff #47994)

Hm, looking at this mangling, the later bit implies the former. Is there a way to rewrite the definition such that the vector type is inferred from the pointer type?

(The code is correct, just slightly verbose.)

include/llvm/IR/IRBuilder.h
515 ↗(On Diff #47994)

Would be good to clarify that the data type is implied by the pointer type and not passed explicitly.

lib/IR/IRBuilder.cpp
218 ↗(On Diff #47994)

You should keep this assert.

While vectorizing, the address space we can keep inside pointer. It should not be a part of the name.
The question only how to specify address space in LLVM IR that use masked intrinsics.
The name like "llvm.masked.store.v2i64.p0v2i64" duplicates information (v2i64) and makes the code unreadable.
The intrinsic with default address space should remain with the short name:
"llvm.masked.store.v2i64"
The intrinsic with specified address space may have one more parameter.
The intrinsic with specified address space may have metadata.
Or look like this: llvm.masked.store.v2i64.a1
There is no way to specify address space in all other intrinsics that use pointers.

delena edited edge metadata.Feb 16 2016, 11:40 PM
delena added a subscriber: igorb.

We have a similar patch in our tree. I didn't see an easy way to add support for mangling only the address space instead of the full pointer type, but I'm not a tblgen expert.

apilipenko added inline comments.Feb 17 2016, 9:33 AM
docs/LangRef.rst
11336 ↗(On Diff #47994)

It seems like in the current scheme we can't have a type dependant on an overloaded type before the overloaded type itself (that's what will happen in the current signature).

Also, once we move to opaque pointer types we will need two overloaded types again.

lib/IR/IRBuilder.cpp
218 ↗(On Diff #47994)

This assert is sank down to CreateMaskedIntrinsic.

Elena - The naming proposed in this patch is the standard mangling for overloaded intrinsics. Most of the options you mentioned involve invention of another mechanism for generic intrinsics or altering of existing one, which is a change of much wider scope. A little bit easier change is introducing another set of intrinsics with both overloaded types in addition to existing ones. But I personally don't think that it worth it.

Also, I expect that once we move to opaque pointer type this redundancy in the naming will go away.

Yes, as Matt mentioned in llvm-dev mail thread there is a bunch of other intrinsic with the same problem. We are specifically concerned about these intrinsics because of the loop-vectorize bug I mentioned in the review description.

You don't need 2 overloaded types. You are duplicating information. llvm.masked.store.v8f64.p0v8f64 : v8f64 appears twice.
If you create name according to pointer, you have llvm.masked.store.p0v8f64.

Elena - see my reply to Philip's comment above, he basically proposed the same. I'll investigate this option, but as is it's not possible to refer to an overloaded type before this type occurs in the signature. In both load/store intrinsics we have a data typed argument before the pointer argument.

To do what you propose we have to have something like that:

def int_masked_store : Intrinsic<[], [LLVMPointerElementType<1>, llvm_anyptr_ty,
                                      llvm_i32_ty,
                                      LLVMVectorSameWidth<0, llvm_i1_ty>],
                                 [IntrReadWriteArgMem]>;

def int_masked_load  : Intrinsic<[LLVMPointerElementType<1>],
                                 [llvm_anyptr_ty, llvm_i32_ty,
                                  LLVMVectorSameWidth<0, llvm_i1_ty>, LLVMMatchType<0>],
                                 [IntrReadArgMem]>;

In both cases we need to match the type of a parameter which is listed after the current parameter. It's not possible in the current scheme.

From Intrinsics.td, class LLVMMatchType comment:

// Match the type of another intrinsic parameter.  Number is an index into the
// list of overloaded types for the intrinsic, excluding all the fixed types.
// The Number value must refer to a previously listed type.

This limitation simplifies parsing of intrinsic definitions, since we always know that the type we are referencing to is already decalred. Support of arbitrary referencing will complicate parsing code in CodeGenTarget a lot. Probably it will require changes to IntrinsicEmitter as well. Not to mention that we don't have a LLVMPointerElementType class in tablgen intrinsics description now.

I don't think that it worth the effort, because eventually when we have typeless pointers we will need both overloaded types again.

theraven added inline comments.
lib/IR/IRBuilder.cpp
216 ↗(On Diff #47994)

Most IRBuilder things that require a pointee type have been moved to take an explicit type, to ease the typeless pointer work. It might be a good idea to make that change here, as this change increases the importance of knowing the type (i.e. add an explicit PtrTy parameter and assert [for now] that it is equal to Ptr->getType())..

reames accepted this revision.Mar 2 2016, 11:31 AM
reames added a reviewer: reames.

I just noticed that this change has no forward serialization for the intrinsics in old bytecode. This will be mandatory for the patch to land. See IR/AutoUpgrade.cpp and the usage of functions such as UpgradeIntrinsicCall in lib/Bitcode/Reader/BitcodeReader.cpp

docs/LangRef.rst
11352 ↗(On Diff #47994)

The additional sentence doesn't add much. In particular, any LLVM pointer can be of arbitrary address space.

include/llvm/IR/IRBuilder.h
515 ↗(On Diff #47994)

doesn't look like this has been addressed - this is a must fix

This revision is now accepted and ready to land.Mar 2 2016, 11:31 AM
reames requested changes to this revision.Mar 2 2016, 11:32 AM
reames edited edge metadata.

Selected wrong menu item - should not be "accepted", meant "request changes".

This revision now requires changes to proceed.Mar 2 2016, 11:32 AM
apilipenko updated this revision to Diff 49741.Mar 3 2016, 7:52 AM
apilipenko edited edge metadata.

Address Philip's comments.

apilipenko added inline comments.Mar 3 2016, 7:53 AM
lib/IR/IRBuilder.cpp
216 ↗(On Diff #49741)

That seems like a good idea, but I'd like to keep the patch as simple as possible, namely I don't want to change IRBuilder API in this change.

reames accepted this revision.Mar 4 2016, 11:22 AM
reames edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 4 2016, 11:22 AM
This revision was automatically updated to reflect the committed changes.