[llvm-commits] [llvm] r172363 - in /llvm/trunk: include/llvm/Analysis/MemoryBuiltins.h lib/Analysis/MemoryBuiltins.cpp
Chandler Carruth
chandlerc at google.com
Sun Jan 13 10:52:10 PST 2013
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.
>
> Modified:
> llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h
> llvm/trunk/lib/Analysis/MemoryBuiltins.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h?rev=172363&r1=172362&r2=172363&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h (original)
> +++ llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h Sun Jan 13 12:02:57
> 2013
> @@ -153,12 +153,14 @@
> class ObjectSizeOffsetVisitor
> : public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetType> {
>
> + typedef DenseMap<const Value*, SizeOffsetType> CacheMapTy;
> +
> const DataLayout *TD;
> const TargetLibraryInfo *TLI;
> bool RoundToAlign;
> unsigned IntTyBits;
> APInt Zero;
> - SmallPtrSet<Value*, 8> SeenInsts;
> + CacheMapTy CacheMap;
>
> APInt align(APInt Size, uint64_t Align);
>
>
> Modified: llvm/trunk/lib/Analysis/MemoryBuiltins.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryBuiltins.cpp?rev=172363&r1=172362&r2=172363&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/MemoryBuiltins.cpp (original)
> +++ llvm/trunk/lib/Analysis/MemoryBuiltins.cpp Sun Jan 13 12:02:57 2013
> @@ -387,17 +387,19 @@
> V = V->stripPointerCasts();
>
> if (isa<Instruction>(V) || isa<GEPOperator>(V)) {
> - // If we have already seen this instruction, bail out.
> - if (!SeenInsts.insert(V))
> - return unknown();
> + // return cached value or insert unknown in cache if size of V was not
> + // computed yet in order to avoid recursions in PHis
>
Comments should be proper prose -- capitalized first letter and a full stop
at the end...
> + 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.
- return Ret;
> + Result = visit(cast<Instruction>(*V));
> + return CacheMap[V] = Result;
> }
>
> if (Argument *A = dyn_cast<Argument>(V))
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130113/4d2cba38/attachment.html>
More information about the llvm-commits
mailing list