llvm dev - Apr 2016 - LTO and intrinsics mangling

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*>)

Cheers,
Rafael

On 20 Apr 2016, at 00:51, Rafael Espíndola <rafael.espindola at
gmail.com<mailto:rafael.espindola at gmail.com>> wrote:

On 18 April 2016 at 19:22, Philip Reames via llvm-dev
<llvm-dev at lists.llvm.org<mailto: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*>)


Cheers,
Rafael

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160420/9d84db93/attachment.html>

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

> 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.  :)
But if it is really an arbitrary suffix, why does it have to be
updated in any fancy way? That is, given a module with

declare <4 x %struct.foo*> @llvm.masked.load.arbitrary_suffix1(<4 x
%struct.foo*>*, i32, <4 x i1>, <4 x %struct.foo*>)

and another with

declare <4 x %struct.bar*> @llvm.masked.load.arbitrary_suffix2(<4 x
%struct.bar*>*, i32, <4 x i1>, <4 x %struct.bar*>)

and given that the linker managed to merge struct.foo and struct.bar,
couldn't we just keep

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.foo*> @llvm.masked.load.arbitrary_suffix2(<4 x
%struct.foo*>*, i32, <4 x i1>, <4 x %struct.foo*>)

in the merged module? That is, we would have two declarations that
will be codegened the same.

Cheers,
Rafael