[llvm-commits] [poolalloc] r159124 - in /poolalloc/trunk: lib/DSA/BottomUpClosure.cpp test/dsa/regression/2012-04-29.SQLiteBUInfiniteRecursion.ll

Will Dietz wdietz2 at illinois.edu
Sun Jun 24 22:02:13 PDT 2012


Author: wdietz2
Date: Mon Jun 25 00:02:13 2012
New Revision: 159124

URL: http://llvm.org/viewvc/llvm-project?rev=159124&view=rev
Log:
Overdue and simple fix for infinite recursion bug in BU.

When deciding to recalculate the DSGraph due to "new knowledge"
(new resolvable callsites introduced while inlining), we only
do so if the new callsites add new callee Functions that weren't
previously being inlined before.

This does mean we'll miss some precision, and to track how often this fires I've
introduced a new statistic that counts when this happens.  Running on CINT2006
this fires exactly once (with almost no effect on the result), so I'm calling it
well worth the potential loss of precision (which seems extraordinarily minor)
for the sake of not going into infinite recursion on some codes.

Note that the unresolved callsites we skip processing will be resolved in the
callers of the current function, so the loss is localized to this particular
graph.

Update the previously XFAIL'd lit test capturing this, it now passes!  Hooray!

For posterity's sake (and perhaps future-Will's sake), I'll add the following:

Ideally this check wouldn't exist, and instead we could recalculate whenever new
callsites were added.  "New" being an interestingly nebulous thing when
callsites are implicitly named by their acyclic call paths, something we do not
explicitly track.  If we did, we should use that to compare if the new callsites
are duplicates of what we had before, but the way we do things now a comparison
of DSCallsites does not capture the call paths used to reach them.

I've sat on this change for quite a while because of this theoretical
deficiency, but I'm tired of DSA crashing on programs due to this and having
measured the resulting precision difference this deviation from 'correct'
doesn't seem to matter.  Hopefully this only a positive effect for DSA and all
of its users.

Onward! :)

Modified:
    poolalloc/trunk/lib/DSA/BottomUpClosure.cpp
    poolalloc/trunk/test/dsa/regression/2012-04-29.SQLiteBUInfiniteRecursion.ll

Modified: poolalloc/trunk/lib/DSA/BottomUpClosure.cpp
URL: http://llvm.org/viewvc/llvm-project/poolalloc/trunk/lib/DSA/BottomUpClosure.cpp?rev=159124&r1=159123&r2=159124&view=diff
==============================================================================
--- poolalloc/trunk/lib/DSA/BottomUpClosure.cpp (original)
+++ poolalloc/trunk/lib/DSA/BottomUpClosure.cpp Mon Jun 25 00:02:13 2012
@@ -32,7 +32,9 @@
   STATISTIC (NumIndResolved, "Number of resolved IndCalls");
   STATISTIC (NumIndUnresolved, "Number of unresolved IndCalls");
   // NumEmptyCalls = NumIndUnresolved + Number of calls to external functions
-  STATISTIC (NumEmptyCalls, "Number of calls we know nothing about"); 
+  STATISTIC (NumEmptyCalls, "Number of calls we know nothing about");
+  STATISTIC (NumRecalculations, "Number of DSGraph recalculations");
+  STATISTIC (NumRecalculationsSkipped, "Number of DSGraph recalculations skipped");
 
   RegisterPass<BUDataStructures>
   X("dsa-bu", "Bottom-up Data Structure Analysis");
@@ -330,6 +332,18 @@
   return;
 }
 
+static bool hasNewCallees(svset<const Function*> &New,
+                          svset<const Function*> &Old) {
+  if (New.size() > Old.size()) return true;
+
+  svset<const Function*>::iterator NI = New.begin(), NE = New.end();
+
+  for (; NI != NE; ++NI)
+    if (!Old.count(*NI)) return true;
+
+  return false;
+}
+
 //
 // Method: calculateGraphs()
 //
@@ -434,14 +448,18 @@
     //
     // Should we revisit the graph?  Only do it if there are now new resolvable
     // callees.
-    getAllAuxCallees(G, CalleeFunctions);
-    if (!CalleeFunctions.empty()) {
-      DEBUG(errs() << "Recalculating " << F->getName() << " due to new knowledge\n");
-      ValMap.erase(F);
-      return calculateGraphs(F, Stack, NextID, ValMap);
-    } else {
-      ValMap[F] = ~0U;
+    FuncSet NewCallees;
+    getAllAuxCallees(G, NewCallees);
+    if (!NewCallees.empty()) {
+      if (hasNewCallees(NewCallees, CalleeFunctions)) {
+        DEBUG(errs() << "Recalculating " << F->getName() << " due to new knowledge\n");
+        ValMap.erase(F);
+        ++NumRecalculations;
+        return calculateGraphs(F, Stack, NextID, ValMap);
+      }
+      ++NumRecalculationsSkipped;
     }
+    ValMap[F] = ~0U;
     return MyID;
   } else {
     unsigned SCCSize = 1;
@@ -492,23 +510,22 @@
     DEBUG(errs() << "  [BU] Done inlining SCC  [" << SCCGraph->getGraphSize()
 	  << "+" << SCCGraph->getAuxFunctionCalls().size() << "]\n"
 	  << "DONE with SCC #: " << MyID << "\n");
-    getAllAuxCallees(SCCGraph, CalleeFunctions);
-    if (!CalleeFunctions.empty()) {
-      DEBUG(errs() << "Recalculating SCC Graph " << F->getName() << " due to new knowledge\n");
-      ValMap.erase(F);
-      return calculateGraphs(F, Stack, NextID, ValMap);
-    } else {
-      ValMap[F] = ~0U;
+    FuncSet NewCallees;
+    getAllAuxCallees(SCCGraph, NewCallees);
+    if (!NewCallees.empty()) {
+      if (hasNewCallees(NewCallees, CalleeFunctions)) {
+        DEBUG(errs() << "Recalculating SCC Graph " << F->getName() << " due to new knowledge\n");
+        ValMap.erase(F);
+        ++NumRecalculations;
+        return calculateGraphs(F, Stack, NextID, ValMap);
+      }
+      ++NumRecalculationsSkipped;
     }
+    ValMap[F] = ~0U;
     return MyID;
   }
 }
 
-  bool compareDSCallSites (const DSCallSite & DS1, const DSCallSite & DS2) {
-    return (DS1.getCallSite().getCalledValue() <
-            DS2.getCallSite().getCalledValue());
-  }
-
 //
 // Method: CloneAuxIntoGlobal()
 //

Modified: poolalloc/trunk/test/dsa/regression/2012-04-29.SQLiteBUInfiniteRecursion.ll
URL: http://llvm.org/viewvc/llvm-project/poolalloc/trunk/test/dsa/regression/2012-04-29.SQLiteBUInfiniteRecursion.ll?rev=159124&r1=159123&r2=159124&view=diff
==============================================================================
--- poolalloc/trunk/test/dsa/regression/2012-04-29.SQLiteBUInfiniteRecursion.ll (original)
+++ poolalloc/trunk/test/dsa/regression/2012-04-29.SQLiteBUInfiniteRecursion.ll Mon Jun 25 00:02:13 2012
@@ -1,5 +1,4 @@
 ;RUN: dsaopt %s -dsa-bu -disable-output
-;XFAIL: *
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-unknown-linux-gnu"
 





More information about the llvm-commits mailing list