Speedup Function::getIntrinsicID() with caching of result
Jean-Luc Duprat
jduprat at apple.com
Fri Mar 1 11:25:08 PST 2013
Will do, thank you all for your help in getting this change in.
JL
On Mar 1, 2013, at 10:52 , Michael Ilseman <milseman at apple.com> wrote:
> r176365
>
> Jean-Luc, for your test commit/first commits, can you do the cleanup Duncan mentioned? Also, as Duncan mentioned, could you add a unit test that checks that the cache behaves properly for Function and Module deletion?
>
> If you look in llvm/unittests, you'll see where you can write C++ code as a unit test.
>
> On Mar 1, 2013, at 12:25 AM, Duncan Sands <baldrick at free.fr> wrote:
>
>> 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
>>>
>>
>
More information about the llvm-commits
mailing list