<div dir="ltr"><div>On Sun, Jan 13, 2013 at 10:02 AM, Nuno Lopes <span dir="ltr"><<a href="mailto:nunoplopes@sapo.pt" target="_blank" class="cremed">nunoplopes@sapo.pt</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: nlopes<br>
Date: Sun Jan 13 12:02:57 2013<br>
New Revision: 172363<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=172363&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=172363&view=rev</a><br>
Log:<br>
fix compile-time regression report by Joerg Sonnenberger:<br>
cache result of Size/OffsetVisitor to speedup analysis of PHI nodes<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified:<br>
llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h<br>
llvm/trunk/lib/Analysis/MemoryBuiltins.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h?rev=172363&r1=172362&r2=172363&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h?rev=172363&r1=172362&r2=172363&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h (original)<br>
+++ llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h Sun Jan 13 12:02:57 2013<br>
@@ -153,12 +153,14 @@<br>
class ObjectSizeOffsetVisitor<br>
: public InstVisitor<ObjectSizeOffsetVisitor, SizeOffsetType> {<br>
<br>
+ typedef DenseMap<const Value*, SizeOffsetType> CacheMapTy;<br>
+<br>
const DataLayout *TD;<br>
const TargetLibraryInfo *TLI;<br>
bool RoundToAlign;<br>
unsigned IntTyBits;<br>
APInt Zero;<br>
- SmallPtrSet<Value*, 8> SeenInsts;<br>
+ CacheMapTy CacheMap;<br>
<br>
APInt align(APInt Size, uint64_t Align);<br>
<br>
<br>
Modified: llvm/trunk/lib/Analysis/MemoryBuiltins.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryBuiltins.cpp?rev=172363&r1=172362&r2=172363&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryBuiltins.cpp?rev=172363&r1=172362&r2=172363&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/MemoryBuiltins.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/MemoryBuiltins.cpp Sun Jan 13 12:02:57 2013<br>
@@ -387,17 +387,19 @@<br>
V = V->stripPointerCasts();<br>
<br>
if (isa<Instruction>(V) || isa<GEPOperator>(V)) {<br>
- // If we have already seen this instruction, bail out.<br>
- if (!SeenInsts.insert(V))<br>
- return unknown();<br>
+ // return cached value or insert unknown in cache if size of V was not<br>
+ // computed yet in order to avoid recursions in PHis<br></blockquote><div><br></div><div>Comments should be proper prose -- capitalized first letter and a full stop at the end...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ std::pair<CacheMapTy::iterator, bool> CacheVal =<br>
+ CacheMap.insert(std::make_pair(V, unknown()));<br>
+ if (!CacheVal.second)<br>
+ return CacheVal.first->second;<br>
<br>
- SizeOffsetType Ret;<br>
+ SizeOffsetType Result;<br>
if (GEPOperator *GEP = dyn_cast<GEPOperator>(V))<br>
- Ret = visitGEPOperator(*GEP);<br>
+ Result = visitGEPOperator(*GEP);<br>
else<br>
- Ret = visit(cast<Instruction>(*V));<br>
- SeenInsts.erase(V);<br></blockquote><div><br></div><div>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.</div><div><br></div><div style>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- return Ret;<br>
+ Result = visit(cast<Instruction>(*V));<br>
+ return CacheMap[V] = Result;<br>
}<br>
<br>
if (Argument *A = dyn_cast<Argument>(V))<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>