[PATCH] D20348: IR: Introduce local_unnamed_addr attribute.

Tue May 31 20:06:03 PDT 2016

On Tue, May 31, 2016 at 7:17 PM Peter Collingbourne <peter at pcc.me.uk> wrote:

> On Tue, May 31, 2016 at 6:36 PM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>>
>> On May 31, 2016, at 6:28 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> On Tue, May 31, 2016 at 6:11 PM Mehdi Amini <mehdi.amini at apple.com>
>> wrote:
>>
>>> On May 31, 2016, at 6:00 PM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>> On Fri, May 27, 2016 at 8:43 AM Mehdi Amini via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> On May 18, 2016, at 11:16 AM, Peter Collingbourne via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, May 17, 2016 at 6:46 PM, Chandler Carruth <chandlerc at gmail.com>
>>>>  wrote:
>>>>
>>>>> On Tue, May 17, 2016 at 6:40 PM Peter Collingbourne via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> On Tue, May 17, 2016 at 6:07 PM, Chandler Carruth <
>>>>>> chandlerc at gmail.com> wrote:
>>>>>>
>>>>>>> Thanks for the detailed write-up, and sorry to Rafael and Mehdi that
>>>>>>> it's on a new thread. =/
>>>>>>>
>>>>>>> On Tue, May 17, 2016 at 5:59 PM Peter Collingbourne via llvm-commits
>>>>>>> <llvm-commits at lists.llvm.org> wrote:
>>>>>>>
>>>>>>>> pcc created this revision.
>>>>>>>> pcc added reviewers: rafael, joker.eph, chandlerc, majnemer.
>>>>>>>> pcc added a subscriber: llvm-commits.
>>>>>>>> Herald added a reviewer: tstellarAMD.
>>>>>>>> Herald added subscribers: jfb, mzolotukhin, joker.eph, arsenm.
>>>>>>>>
>>>>>>>> If a local_unnamed_addr attribute is attached to a global, the
>>>>>>>> address
>>>>>>>> is known to be insignificant within the module. It is distinct from
>>>>>>>> the
>>>>>>>> existing unnamed_addr attribute in that it only describes a local
>>>>>>>> property
>>>>>>>> of the module rather than a global property of the symbol.
>>>>>>>>
>>>>>>>> This attribute is intended to be used by the code generator and LTO
>>>>>>>> to allow
>>>>>>>> the linker to decide whether the global needs to be in the symbol
>>>>>>>> table. It is
>>>>>>>> possible to exclude a global from the symbol table if three things
>>>>>>>> are true:
>>>>>>>> - This attribute is present on every instance of the global (which
>>>>>>>> means that
>>>>>>>>   the normal rule that the global must have a unique address can
>>>>>>>> be broken without
>>>>>>>>   being observable by the program by performing comparisons
>>>>>>>> against the global's
>>>>>>>>   address)
>>>>>>>> - The global has linkonce_odr linkage (which means that each
>>>>>>>> linkage unit must have
>>>>>>>>   its own copy of the global if it requires one, and the copy in
>>>>>>>> each linkage unit
>>>>>>>>   must be the same)
>>>>>>>> - It is a constant or a function (which means that the program
>>>>>>>> cannot observe that
>>>>>>>>   the unique-address rule has been broken by writing to the global)
>>>>>>>>
>>>>>>>> Although this attribute could in principle be computed from the
>>>>>>>> module
>>>>>>>> contents, LTO clients (i.e. linkers) will normally need to be able
>>>>>>>> to compute
>>>>>>>> this property as part of symbol resolution, and it would be
>>>>>>>> inefficient to
>>>>>>>> materialize every module just to compute it.
>>>>>>>>
>>>>>>>
>>>>>>> Cool, this last part is really key.
>>>>>>>
>>>>>>>
>>>>>>> My real problem with adding this as a normal attribute is that I'm
>>>>>>> not sure what it really means. Is it just a "cache" of some local analysis?
>>>>>>> Do we expect things to invalidate it if they make the address significant
>>>>>>> within a module? Is this something that would be "blessed" by some
>>>>>>> frontends?
>>>>>>>
>>>>>>> I feel like, from your description, this really is just intended to
>>>>>>> solve the problem of materializing all of the module. It would seem that
>>>>>>> for that purpose something more akin to the "summary" information used by
>>>>>>> ThinLTO would be a better tool than an attribute which has to have a
>>>>>>> semantic contract for the IR.
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>
>>>>>> To a certain extent this is a summary of the module contents, and in
>>>>>> most cases I'd expect the property to simply summarize the module.
>>>>>>
>>>>>> However, it is also a property that should be preserved if a pass
>>>>>> introduces an address comparison. Modulo bugs, the new comparison should be
>>>>>> "benign" -- if the original program could not observe the address, the
>>>>>> optimized program shouldn't be able to observe it either.
>>>>>>
>>>>>> One example of this would be a comparison of a vptr against a vtable
>>>>>> or function address for speculative devirtualization, or in general any
>>>>>> form of PGO that relies on global addresses. The introduction of such a
>>>>>> comparison doesn't invalidate the unnamed_addr property (which we do
>>>>>> currently apply to vtables), as the program would still have the same
>>>>>> semantics if we, say, merged two identical vtables. The same applies to the
>>>>>> local_unnamed_addr property.
>>>>>>
>>>>>>  Regarding frontends, yes, I'd expect that if a frontend knows that
>>>>>> all address comparisons within a module are benign, it could apply this
>>>>>> property.
>>>>>>
>>>>>
>>>>> OK, all of this argues that we *can* define this as a semantic
>>>>> attribute, but doesn't really speak to why we *should*.
>>>>>
>>>>
>>>>
>>>> As Peter mentioned above, some transformations can introduce constructs
>>>> that would prevent from inferring the attribute, which means that when
>>>> writing out bitcode we won't be able to generate it.  Because of that,
>>>> having it only in the bitcode (like the summary) makes it more "fragile"
>>>> (round-tripping may be broken).
>>>>
>>>> It seems to me that if we don't make it first class in the IR, we will
>>>> still want to have an analysis (or metadata) populated from the summary
>>>> data that can be used to preverse this "property".
>>>>
>>>
>>> An analysis to allow easy access makes perfect sense to me, and even
>>> something more akin to metadata might make sense.
>>>
>>>
>>> I don't see the fundamental difference between a metadata and an
>>> attribute when you start to attach it some semantic, it looks like an
>>> implementation detail to me. Maybe the difference is that we should always
>>> be able to drop a metadata without correctness issue?
>>>
>>
>> That combined with the specific mandate to actively drop metadata when
>> transforming code unless you know how to update it correctly.
>>
>> However, the fact that we fail in practice to do that quite often speaks
>> to why a constructively correct design would be better IMO.
>>
>>
>>> But it seems to me that this would apply to multiple existing attributes
>>> as well.
>>>
>>
>> I agree. There are a number of attributes that would be better served by
>> having improved ability to run interprocedural analyses and cache their
>> results. But I'd like to not make the situation worse. As I've mentioned, I
>> think we have become a bit too willing to introduce attributes to LLVM's
>> IR. Their cost often ends up being surprisingly large, and I suspect we
>> should be more reluctant to accept them (where we can).
>>
>>
>> I agree (that we should be more reluctant).
>>
>>
>>
>>>
>>>
>>>
>>> I'll try to explain why an IR attribute seems strange to me: we have to
>>> remember for all time to update it.
>>>
>>> I think that IR attributes which only *reflect* or "cache" the state of
>>> the program itself make every transformation which could possibly
>>> invalidate them fragile. We have to continually remember the set of
>>> attributes to go and invalidate. Attributes seem much more important when
>>> they *promise* some state that may not be (re-)computable, and thus it is
>>> *necessary* to have a fundamental semantic bit to indicate that the
>>> property must hold. Even better when the properties themselves are
>>> inherently defined in a way that isn't invalidated by transformations.
>>>
>>>
>>> Isn't it exactly the case here?
>>> Transformations will not invalidate it "conceptually" (the attribute
>>> would always be valid and we don't need to care about invalidating it), but
>>> on the other side transformations can turn the code into a form where we
>>> can't deduce the attribute anymore (i.e. what you described IIUC as 'we
>>> "promise"  that may not be (re-)computable').
>>>
>>
>> This is not the impression I was left with -- I was left specifically
>> with the impression that the usage of the attribute would require that the
>> implementation match.
>>
>> If that isn't correct, that would at least go some way to making me more
>> comfortable with this as an attribute.
>>
>>
>> That's not my understanding, Peter may be able to confirm?
>>
>
> Yes, I think there might have been a misunderstanding here. The
> local_unnamed_addr attribute (and unnamed_addr) both reflect a property of
> the source code. The IR does not have to match that attribute, as
> transformations may introduce comparisons, which must be unobservable (per
> the general principle that passes cannot change observable semantics), and
> as such do not invalidate the attribute.
>

OK, that's definitely something I misunderstood then. If this is primarily
a property of the source code, I'm much more comfortable with it. And I can
also see how it would almost have to be, as otherwise I fear it would run
afoul of the derefinement problem that Sanjoy has been formalizing.

However, this makes me wonder something new, see below...

>
> I'd expect any transformation that really invalidates this attribute to
>> also invalidate unnamed_addr somehow. And while some transformations can
>> prevents from computing it (by introducing a comparison on the address when
>> messing with loops for instance), they don't really escape the address and
>> I don't think any transform is invalidating unnamed_addr today.
>>
>
> Exactly, the only pass we have in the tree that "invalidates" unnamed_addr
> is the constant merge pass [1], which has legitimate reasons for doing so
> since it is merging source-level entities.
>

Interesting. And I assume it would also break local_unnamed_addr and you
would want it to strip that?

This and the above really makes me wonder -- why do we need two attributes
here? Do we really have two separate models?

The rules for unnamed_addr are that if you merge a non-unnamed_addr global
and an unnamed_addr global, you get a non-unnamed_addr global. Why not
apply that exact rule to merging the *same* global during linking of
modules? The result, it seems to me, would be precisely the semantics for
your local_unnamed_addr. I can't off hand think of a use case for one that
couldn't be satisfied by the other, but it is entirely possible I'm missing
the distinction. =] If I am, it would be good to clarify here (and in the
documentation for the attribute)

Also, thanks very much for digging into these issues (all of you), as I
think we're getting increasingly on the same page about the semantic model
being introduced here. Hopefully much of this can be distilled and captured
in documentation as it converges.

-Chandler

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160601/95e9cf8f/attachment.html>