[llvm] r230835 - [RewriteStatepointsForGC] Fix another order of iteration bug

Philip Reames listmail at philipreames.com
Fri Feb 27 17:52:09 PST 2015


Author: reames
Date: Fri Feb 27 19:52:09 2015
New Revision: 230835

URL: http://llvm.org/viewvc/llvm-project?rev=230835&view=rev
Log:
[RewriteStatepointsForGC] Fix another order of iteration bug

It turns out the naming of inserted phis and selects is sensative to the order in which two sets are iterated.  We need to nail this down to avoid non-deterministic output and possible test failures.  

The modified test is the one I first noticed something odd in.  The change is making it more strict to report the error.  With the test change, but without the code change, the test fails roughly 1 in 5.  With the code change, I've run ~30 runs without error.

Long term, the right fix here is to adjust the naming scheme.  I'm checking in this hack to avoid any possible non-determinism in the tests over the weekend.  HJust because I only noticed one case doesn't mean it's actually the only case.  I hope to get to the right change Monday.

std->llvm data structure changes bugfix change #3


Modified:
    llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
    llvm/trunk/test/Transforms/RewriteStatepointsForGC/base-pointers.ll

Modified: llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230835&r1=230834&r2=230835&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp Fri Feb 27 19:52:09 2015
@@ -776,10 +776,20 @@ static Value *findBasePointer(Value *I,
   }
 
   // Insert Phis for all conflicts
-  // Only changing keys in 'states', thus safe to keep iterators
+  // We want to keep naming deterministic in the loop that follows, so
+  // sort the keys before iteration.  This is useful in allowing us to
+  // write stable tests. Note that there is no invalidation issue here.
+  SmallVector<Value*, 16> Keys;
+  Keys.reserve(states.size());
   for (auto Pair : states) {
-    Instruction *v = cast<Instruction>(Pair.first);
-    PhiState state = Pair.second;
+    Value *V = Pair.first;
+    Keys.push_back(V);
+  }
+  std::sort(Keys.begin(), Keys.end(), order_by_name);
+  // TODO: adjust naming patterns to avoid this order of iteration dependency
+  for (Value *V : Keys) {
+    Instruction *v = cast<Instruction>(V);
+    PhiState state = states[V];
     assert(!isKnownBaseResult(v) && "why did it get added?");
     assert(!state.isUnknown() && "Optimistic algorithm didn't complete!");
     if (!state.isConflict())
@@ -974,7 +984,13 @@ static void findBasePointers(const State
                              DenseMap<llvm::Value *, llvm::Value *> &PointerToBase,
                              DominatorTree *DT, DefiningValueMapTy &DVCache,
                              DenseSet<llvm::Value *> &NewInsertedDefs) {
-  for (Value *ptr : live) {
+  // For the naming of values inserted to be deterministic - which makes for
+  // much cleaner and more stable tests - we need to assign an order to the
+  // live values.  DenseSets do not provide a deterministic order across runs.
+  SmallVector<Value*, 64> Temp;
+  Temp.insert(Temp.end(), live.begin(), live.end());
+  std::sort(Temp.begin(), Temp.end(), order_by_name);
+  for (Value *ptr : Temp) {
     Value *base = findBasePointer(ptr, DVCache, NewInsertedDefs);
     assert(base && "failed to find base pointer");
     PointerToBase[ptr] = base;

Modified: llvm/trunk/test/Transforms/RewriteStatepointsForGC/base-pointers.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/RewriteStatepointsForGC/base-pointers.ll?rev=230835&r1=230834&r2=230835&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/RewriteStatepointsForGC/base-pointers.ll (original)
+++ llvm/trunk/test/Transforms/RewriteStatepointsForGC/base-pointers.ll Fri Feb 27 19:52:09 2015
@@ -76,7 +76,7 @@ loop:
 ; CHECK-LABEL: loop
 ; CHECK:   %base_phi = phi i64 addrspace(1)*
 ; CHECK-DAG: [ %base_obj, %entry ]
-; CHECK-DAG: [ %base_select
+; CHECK-DAG: [ %base_select.relocated, %loop ]
 ; CHECK-NOT: base_phi2
 ; CHECK: next = select
 ; CHECK: base_select





More information about the llvm-commits mailing list