Speedup Function::getIntrinsicID() with caching of result

[Duncan Sands]

Hi Jean-Luc,

On 28/02/13 23:43, Jean-Luc Duprat wrote:
> Attached is the revised patch, taking into account all the feedback received.

--- lib/IR/Module.cpp	(revision 176302)
+++ lib/IR/Module.cpp	(working copy)
@@ -13,6 +13,7 @@

  #include "llvm/IR/Module.h"
  #include "SymbolTableListTraitsImpl.h"
+#include "LLVMContextImpl.h"
  #include "llvm/ADT/DenseSet.h"
  #include "llvm/ADT/STLExtras.h"
  #include "llvm/ADT/SmallString.h"

I guess this bit isn't needed any more.  Also, did you check that when a module
is deleted, the cache is cleared (i.e. that function destructors are run).

Ciao, Duncan.

>
> Thank you for your comments,
>
> JL
>
>
>
>
>
>
>
> 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
>