[PATCH] D20348: IR: Introduce local_unnamed_addr attribute.

Tue May 31 21:48:11 PDT 2016

On Tue, May 31, 2016 at 8:06 PM, Chandler Carruth <chandlerc at google.com>
wrote:

> 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?
>

Yes.

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

Well, unnamed_addr does have stronger semantics, and some passes do take
advantage of them. lld does currently use unnamed_addr in the way you
describe, but it causes us to export at least 2624 more symbols than
necessary (see pr27553).

Replacing unnamed_addr with a separate attribute with local_unnamed_addr
semantics is an interesting possibility, but I'm not sure if it's feasible.
See my response to Mehdi.

Peter

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

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160531/a8a2989d/attachment.html>