[llvm-dev] LTO and intrinsics mangling

[Philip Reames via llvm-dev]

On 04/19/2016 02:51 PM, Rafael Espíndola wrote:
> On 18 April 2016 at 19:22, Philip Reames via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>>
>> On 04/18/2016 09:45 AM, Artur Pilipenko via llvm-dev wrote:
>>
>> In the current mangling scheme for overloaded intrinsics we include
>> overloaded type names in the intrinsic name. For example:
>>
>> %struct.foobar = type { i32 }
>> declare <4 x %struct.foobar*> @llvm.masked.load.v4p0struct.foobar(<4 x
>> %struct.foobar*>*, i32, <4 x i1>, <4 x %struct.foobar*>)
>>
>> Verifier checks that an overloaded intrinsic name matches with its
>> signature.
>>
>> When different modules are loaded in LTO configuration with the same
>> LLVMContext, types with the same name from different modules are renamed so
>> their names are unique (%struct.foobar in the second module becomes
>> %struct.foobar.0). After renaming intrinsic names and signatures can become
>> inconsistent.
>>
>> This seems like a clear bug in the module loading.  If we're changing type
>> names, we need to change the intrinsic signatures as well.
>>
>>
>> Usually it slips unnoticed because we don't verify individual modules and
>> eventually map isomorphic types to a single type. So isomorphic types get
>> their original names. Although in some cases it causes problems.
>>
>> Initially I came across the problem with my recent change which added an
>> overloaded type to the masked load/store intrinsics
>> (http://reviews.llvm.org/D17270). The discrepancy between the name and the
>> signature triggers auto-upgrade bit from my patch converting an incorrect
>> mangling to the correct one. But later after remapping of isomorphic types
>> when we return to the original type name this “updated" intrinsic name
>> become invalid.
>>
>> Another way to trigger the problem is to have different types with the same
>> name in different modules. Corresponding test case is attached. In this case
>> types in different modules will be renamed but the intrinsics from different
>> modules will have the same name which will be caught by verifier.
>>
>> As a possible solution we can use AutoUpgrade to handle the situation when
>> the name of the intrinsic doesn't match with its signature. In such cases we
>> have to rename the intrinsic. Then during linking if we map some isomorphic
>> types we have to update intrinsics names. To do that we have to teach
>> IRMover to update intrinsics signatures according to the types mapping.
>>
>> Does this sound reasonable? Are there any other alternatives?
>>
>> Given our current intrinsic naming scheme, the approach you've described
>> seems entirely reasonable.
>>
>> An alternate scheme would be to make the intrinsic signatures insensitive to
>> the struct naming.  (That was probably a bad idea on my part to start with,
>> sorry!)  I would argue that we should not block your original change on a
>> re-architecting effort here.
> I sorry I missed this when it first went in. In general the name of
> llvm types should not be significant. What was the motivation for
> having it in the name of the intrinsic? Could we change to using
>
> declare <4 x %struct.foo*> @llvm.masked.load.arbitrary_suffix1(<4 x
> %struct.foo*>*, i32, <4 x i1>, <4 x %struct.foo*>)
>
> declare <4 x %struct.bar*> @llvm.masked.load.arbitrary_suffix2(<4 x
> %struct.bar*>*, i32, <4 x i1>, <4 x %struct.bar*>)
The name of the struct is used in forming the arbitrary (unique) 
suffix.  It's essentially a hash(types) with a hash function intended to 
be somewhat human readable.  :)
>
> Cheers,
> Rafael