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

[Philip Reames via llvm-commits]

On 01/27/2016 09:34 AM, Mehdi Amini wrote:
>> On Jan 8, 2016, at 2:09 PM, Philip Reames <listmail at philipreames.com> wrote:
>>
>>
>>
>> On 01/08/2016 01:26 PM, Mehdi Amini wrote:
>>>> On Jan 8, 2016, at 12:26 PM, Philip Reames <listmail at philipreames.com> wrote:
>>>>
>>>>
>>>>
>>>> On 01/08/2016 12:06 PM, Mehdi Amini wrote:
>>>>>> 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 (?).
>>>> Hm, this is a good point.  Sorry I misrepresented what the change did.  I focused on the new code and didn't pay too much attention to what the old code did ownership wise.
>>>>
>>>> I still don't like this structure, but I do see your point now about it not making things worse than they already were.  Given this, can I ask you to switch to AssertingVH?  I'll otherwise withdraw my objections.
>>>>>> 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 2 actually doesn't work.  It would remove the locking on the string table, but not on the map of functions to strings.  So, disregard.
>>>>> - 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.
>>>> I agree that the GC attribute code is a bit crufty in general.  It's on my list of things to address someday, but I haven't gotten there yet.  With the current structure, you can't really avoid the locking at some point.
>>>>
>>>> Hm, another possible design would be to assign each encountered string a small integer value.  Mapping from string to integer would require a reader/writer biased lock, but the actual small integer could be represent in the SubClassData by stealing an extra bit or two.
>>>>> Why isn’t this a function attribute instead?
>>>> Not entirely sure.  I think this is mostly a historical accident at this point.
>>> Any reason not to move it to a function attribute now then?
>> Other than the needed forward bitcode compatibility, not that I know of.
> I had a look at it last week, and it seems to me that one major difference is that the string attribute wouldn’t be pooled like it is now and the string would be duplicated for each function.
> It may be possible to have a custom attribute type to pool the string, but it seems like not trivial work.
> Thoughts?
This sounds like a deficiency in our handler of string attributes. If we 
have the same string attribute on each function in a module, it's be 
nice to be able to share the storage in the same way.

Now, I have no evidence the pooling is important for gc performance.  It 
might be simplest to just remove the pooling and then enhance string 
attributes as a whole later.

Philip