Speedup Function::getIntrinsicID() with caching of result

[Jean-Luc Duprat]

The final performance numbers with the patch applied.

Before this change:
$ time ./bin/llc  -march=arm -mattr=+neon vsub_JLD2.ll -O0
real	0m10.995s
$ time ./bin/llc  -march=arm -mattr=+neon vsub_JLD2.ll -O2
real	0m11.894s
$ time ./bin/opt -O2 -S -instcombine vsub_JLD2.ll > /dev/null
real	0m0.925s

After this change:
$ time ./bin/llc  -march=arm -mattr=+neon vsub_JLD2.ll -O0
real	0m10.440s (-5%)
$ time ./bin/llc  -march=arm -mattr=+neon vsub_JLD2.ll -O2
real	0m11.383s (-5%)
$ time ./bin/opt -O2 -S -instcombine vsub_JLD2.ll> /dev/null
real	0m0.241s (-74%)

I will check in the unit test this afternoon.

JL


On Mar 1, 2013, at 11:25 , Jean-Luc Duprat <jduprat at apple.com> wrote:

> 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
>>>> 
>>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits