[PATCH] D20348: IR: Introduce local_unnamed_addr attribute.

Tue May 31 20:47:31 PDT 2016

> On 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 <mailto:peter at pcc.me.uk>> wrote:
> On Tue, May 31, 2016 at 6:36 PM, Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
> 
>> On May 31, 2016, at 6:28 PM, Chandler Carruth <chandlerc at google.com <mailto:chandlerc at google.com>> wrote:
>> 
>> On Tue, May 31, 2016 at 6:11 PM Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>>> On May 31, 2016, at 6:00 PM, Chandler Carruth <chandlerc at google.com <mailto: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 <mailto: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 <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> 
>>>> 
>>>> 
>>>> On Tue, May 17, 2016 at 6:46 PM, Chandler Carruth <chandlerc at gmail.com <mailto: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 <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> On Tue, May 17, 2016 at 6:07 PM, Chandler Carruth <chandlerc at gmail.com <mailto: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 <mailto: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)

Assuming that we can't have non-local global with unnamed_addr (LangRef is not clear on this, but I'm not sure it would really make sense?), It is possible that unnamed_addr is identical to "local_unnamed_addr + local linkage", in which case we could ditch the unnamed_addr attribute and replace the hasUnnamedAddr() predicate to be:

bool GlobalValue::hasUnnamedAddr() {
  return hasLocalUnnamedAddr() && hasLocalLinkage();
}

-- 
Mehdi

> 
> 
> 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/20160531/6c693879/attachment.html>