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

[Chandler Carruth]

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>