[llvm-commits] [llvm] r172363 - in /llvm/trunk: include/llvm/Analysis/MemoryBuiltins.h lib/Analysis/MemoryBuiltins.cpp

[Nuno Lopes]

> On Sun, Jan 13, 2013 at 10:02 AM, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>
>> Author: nlopes
>> Date: Sun Jan 13 12:02:57 2013
>> New Revision: 172363
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=172363&view=rev
>> Log:
>> fix compile-time regression report by Joerg Sonnenberger:
>> cache result of Size/OffsetVisitor to speedup analysis of PHI nodes
>>
>
> I suspect this wasn't a performance issue so much as a termination issue.
> The test case Joerg had inf-looped for me, and if I removed the 'erase'
> from the set, completed very rapidly.
>
> Please reduce the test case from Joerg to an .ll file and include it as a
> regression test, it should be a very useful one for subsequent changes.

I don't think it was going into an infinite loop, otherwise the stack would 
grow too much and crash. I didn't wait to see the test finishing, but I have 
a very slow computer anyway :)


>> +    std::pair<CacheMapTy::iterator, bool> CacheVal =
>> +      CacheMap.insert(std::make_pair(V, unknown()));
>> +    if (!CacheVal.second)
>> +      return CacheVal.first->second;
>>
>> -    SizeOffsetType Ret;
>> +    SizeOffsetType Result;
>>      if (GEPOperator *GEP = dyn_cast<GEPOperator>(V))
>> -      Ret = visitGEPOperator(*GEP);
>> +      Result = visitGEPOperator(*GEP);
>>      else
>> -      Ret = visit(cast<Instruction>(*V));
>> -    SeenInsts.erase(V);
>>
>
> Did you try just removing this erase? That was enough to fix the inf-loop
> for me, but it caused one regression test to fail.
>
> I mention this at all because the map is significantly more expensive than
> the small pointer set -- if there is a way to get this to work with just
> the visited set that would seem superior. But I don't know a lot of this
> code, so maybe there are other constraints.

Removing that erase is not an option, since it would break functionality. 
Sometimes we visit the same pointer if e.g. the PHI income values are not 
distinct.  The map also helps in that case since the analysis is now cached.
Anyway, my quick profiling showed that this function is responsible by less 
than 0.5% of compile time.


Nuno