[PATCH] D20348: IR: Introduce local_unnamed_addr attribute.

[Chandler Carruth via llvm-commits]

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

Adding yet another semantic attribute is a really invasive change, and it
feels like there should be a smaller / simpler mechanism to achieve the
goals you have here. The whole point of having summary information
available it bitcode was to allow LTO-like applications to read a small
header to answer specific questions like the one motivating this patch.

I'd like to understand why a summary approach isn't the correct approach
here before we extend the IR to represent a new concept, especially one
with extremely close overlap and subtle distinctions from an existing
concept.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160518/fabf573c/attachment.html>