[llvm] r257139 - Remove static global GCNames from Function.cpp and move it to the Context

[Mehdi Amini via llvm-commits]

> On Jan 8, 2016, at 11:52 AM, Philip Reames <listmail at philipreames.com> wrote:
> 
> 
> 
> On 01/08/2016 11:40 AM, Mehdi Amini wrote:
>>> On Jan 8, 2016, at 11:37 AM, Philip Reames <listmail at philipreames.com> wrote:
>>> 
>>> I'm really not sure this patch is a good idea.  Introducing a mapping of Functions inside the LLVMContext doesn't seem like the best idea from an ownership perspective.  Putting something like this in the Module might make sense - since the Module owns the Function - but bare pointers in a long lived structure to shorter lived objects is a bad idea.
>> The pointer are removed from the context when a function is destroyed.
>> There is no increased lifetime as a result of this patch.
> This is non obvious from the code.  As in, when I read the code, I didn't see this actually happening.  I reread the code and you're correct, but its definitely not obvious.
>> 
>>> Please revert this patch.
>> It seems like a strict improvement over the previous situation, why would you want to revert this?
> Because you are introducing raw pointers and subverting the ownership relations.  I have no problem with the goal in mind - I approve actually - but the structure of this patch is not appropriate.

Assuming you are talking about the “Function *” in the map in the context, I don’t *introduce* raw pointer, I moved this map that was a global variable to be living on the Context. I tried to mostly do pure code motion here: there was a mutated global map, I moved it to the context.
The underlying lifetime change in the patch is that instead of a global map of Function -> string there is now a per context map. So the map is created/destroyed with the context instead of being created the first time you set a GC on a function, and never destroyed (?).


> A couple of possible options:
> - Move this structure to the Module and use AssertingVH.
> - Bake in the 5 or so known names into the table so that we can avoid the locking for GCs shipped with LLVM.
> - Add a bailout in clearGC which early exits without locking if this function doesn't have GC.  In practice, this would completely eliminate the problem.  Using the extra bit of SubClassData would be perfect for this.
> 
> I would prefer something along the lines of the later two options.

I don’t know anything about how this GC attribute is used, but:
- option 1 is fine to me, 
- option 2 means you lock statically the list of possible string?
- option 3 means you introduce again a mutated global behind a lock. I understand it solves my performance issue but it doesn’t seem like a good design usually.

Why isn’t this a function attribute instead?

— 
Mehdi