Speedup Function::getIntrinsicID() with caching of result

[Jean-Luc Duprat]

Attached is the revised patch, taking into account all the feedback received.

Thank you for your comments,

JL


-------------- next part --------------
A non-text attachment was scrubbed...
Name: getIntrinsicID.patch
Type: application/octet-stream
Size: 5505 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130228/e4871048/attachment.obj>
-------------- next part --------------



On Feb 28, 2013, at 10:03 AM, Jean-Luc Duprat <jduprat at apple.com> wrote:

> As discussed, I will replace Module's destructor clear() call with a call to erase() in Function's destructor.
> Thank you for the feedback, I'll send a revised patch out soon.
> 
> JL
> 
> 
> 
> On Feb 28, 2013, at 09:59 , Michael Ilseman <milseman at apple.com> wrote:
> 
>> 
>> On Feb 28, 2013, at 9:34 AM, Duncan Sands <baldrick at free.fr> wrote:
>> 
>>> Hi Michael,
>>> 
>>> On 28/02/13 18:25, Michael Ilseman wrote:
>>>> 
>>>> On Feb 28, 2013, at 2:21 AM, Duncan Sands <baldrick at free.fr> wrote:
>>>> 
>>>>> Hi Jean-Luc,
>>>>> 
>>>>> On 28/02/13 02:16, Jean-Luc Duprat wrote:
>>>>>> The attached patch caches the result of Function::getIntrinsicID() in a DenseMap attached to the LLVMContext.  This reduces the time actually spent doing string to ID conversion and shows a 10% improvement in compile time for a particularly bad case that involves ARM Neon intrinsics (these have many overloads).
>>>>>> 
>>>>>> This changes passes the regression tests and the nightly tests suite.
>>>>> 
>>>>> if you delete an intrinsic function declaration from the module, don't you get a
>>>>> stale pointer to freed memory in the cache?  To avoid all such issues, maybe it
>>>>> is simpler to have the cache map the name (a StringRef) to the intrinsic ID,
>>>>> rather than mapping the function pointer to the intrinsic ID.  You probably lose
>>>>> some of the speedup then though.
>>>> 
>>>> Can he instead have the destructor remove itself from the cache? This would preserve the speedups of key-ing off of the Function pointer directly.
>>> 
>>> this would also mean that the code that clears the cache when a module is
>>> deleted wouldn't be needed.
>> 
>> Yes, I think the replacement is needed.
>> 
>>> 
>>>> 
>>>> Also, since it's the Function* that's the key, I believe the situation you described wouldn't provide an error so much as it would under-utilize the map by having extra meaningless entries. However, maybe there would be an error if anyone ever iterated over the cache. Replacing the Module's destructor clear() call with a Function's destructor erase() call might be a good way to go.
>>> 
>>> The problem is if that freed memory gets reallocated for another function.  Then
>>> the cache lookup will wrongly think that the new function has the intrinsic id
>>> of the freed function.
>>> 
>> 
>> Ah, yes, of course.
>> 
>> Jean-Luc, do you think you could replace Module's destructor clear() call with a call to erase() in Function's destructor?
>> 
>>> Ciao, Duncan.
>>> 
>>>> 
>>>>> 
>>>>> Ciao, Duncan.
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits