fix dce bug with not updating the parent when turning a node unreachable (#1198)

kripken · web-flow · commit c69253378014 · 2017-09-25T14:36:32.000-07:00
diff --git a/src/ast/type-updating.h b/src/ast/type-updating.h
@@ -75,10 +75,16 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
 
   // note the replacement of one node with another. this should be called
   // after performing the replacement.
-  // this does *not* look into the node. you should do so yourself if necessary
-  void noteReplacement(Expression* from, Expression* to) {
+  // this does *not* look into the node by default. see noteReplacementWithRecursiveRemoval
+  // (we don't support recursive addition because in practice we do not create
+  // new trees in the passes that use this, they just move around children)
+  void noteReplacement(Expression* from, Expression* to, bool recursivelyRemove=false) {
     auto parent = parents[from];
-    noteRemoval(from);
+    if (recursivelyRemove) {
+      noteRecursiveRemoval(from);
+    } else {
+      noteRemoval(from);
+    }
     // if we are replacing with a child, i.e. a node that was already present
     // in the ast, then we just have a type and parent to update
     if (parents.find(to) != parents.end()) {
@@ -91,6 +97,10 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
     }
   }
 
+  void noteReplacementWithRecursiveRemoval(Expression* from, Expression* to) {
+    noteReplacement(from, to, true);
+  }
+
   // note the removal of a node
   void noteRemoval(Expression* curr) {
     noteRemovalOrAddition(curr, nullptr);
diff --git a/src/passes/DeadCodeElimination.cpp b/src/passes/DeadCodeElimination.cpp
@@ -48,6 +48,7 @@ struct DeadCodeElimination : public WalkerPass<PostWalker<DeadCodeElimination>>
 
   Expression* replaceCurrent(Expression* expression) {
     auto* old = getCurrent();
+    if (old == expression) return expression;
     WalkerPass<PostWalker<DeadCodeElimination>>::replaceCurrent(expression);
     // also update the type updater
     typeUpdater.noteReplacement(old, expression);
@@ -219,14 +220,17 @@ struct DeadCodeElimination : public WalkerPass<PostWalker<DeadCodeElimination>>
   }
 
   static void scan(DeadCodeElimination* self, Expression** currp) {
+    auto* curr = *currp;
     if (!self->reachable) {
-      // convert to an unreachable. do this without UB, even though we have no destructors on AST nodes
+      // convert to an unreachable safely
       #define DELEGATE(CLASS_TO_VISIT) { \
-        self->typeUpdater.noteRecursiveRemoval(*currp); \
-        ExpressionManipulator::convert<CLASS_TO_VISIT, Unreachable>(static_cast<CLASS_TO_VISIT*>(*currp)); \
+        auto* parent = self->typeUpdater.parents[curr]; \
+        self->typeUpdater.noteRecursiveRemoval(curr); \
+        ExpressionManipulator::convert<CLASS_TO_VISIT, Unreachable>(static_cast<CLASS_TO_VISIT*>(curr)); \
+        self->typeUpdater.noteAddition(curr, parent); \
         break; \
       }
-      switch ((*currp)->_id) {
+      switch (curr->_id) {
         case Expression::Id::BlockId: DELEGATE(Block);
         case Expression::Id::IfId: DELEGATE(If);
         case Expression::Id::LoopId: DELEGATE(Loop);
@@ -256,7 +260,6 @@ struct DeadCodeElimination : public WalkerPass<PostWalker<DeadCodeElimination>>
       #undef DELEGATE
       return;
     }
-    auto* curr =* currp;
     if (curr->is<If>()) {
       self->pushTask(DeadCodeElimination::doVisitIf, currp);
       if (curr->cast<If>()->ifFalse) {
@@ -328,18 +331,18 @@ struct DeadCodeElimination : public WalkerPass<PostWalker<DeadCodeElimination>>
     for (size_t i = 0; i < list.size(); ++i) {
       auto* elem = list[i];
       if (isUnreachable(elem)) {
-	auto* replacement = elem;
-	if (i > 0) {
-	  auto* block = getModule()->allocator.alloc<Block>();
-	  for (size_t j = 0; j < i; ++j) {
-	    block->list.push_back(drop(list[j]));
-	  }
-	  block->list.push_back(list[i]);
-	  block->finalize(type);
-	  replacement = block;
-	}
-	replaceCurrent(replacement);
-	return;
+        auto* replacement = elem;
+        if (i > 0) {
+          auto* block = getModule()->allocator.alloc<Block>();
+          for (size_t j = 0; j < i; ++j) {
+            block->list.push_back(drop(list[j]));
+          }
+          block->list.push_back(list[i]);
+          block->finalize(type);
+          replacement = block;
+        }
+        replaceCurrent(replacement);
+        return;
       }
     }
   }
@@ -349,7 +352,7 @@ struct DeadCodeElimination : public WalkerPass<PostWalker<DeadCodeElimination>>
   }
 
   void visitLoad(Load* curr) {
-    blockifyReachableOperands({ curr->ptr}, curr->type);
+    blockifyReachableOperands({ curr->ptr }, curr->type);
   }
 
   void visitStore(Store* curr) {
@@ -369,11 +372,11 @@ struct DeadCodeElimination : public WalkerPass<PostWalker<DeadCodeElimination>>
   }
 
   void visitBinary(Binary* curr) {
-    blockifyReachableOperands({ curr->left, curr->right}, curr->type);
+    blockifyReachableOperands({ curr->left, curr->right }, curr->type);
   }
 
   void visitSelect(Select* curr) {
-    blockifyReachableOperands({ curr->ifTrue, curr->ifFalse, curr->condition}, curr->type);
+    blockifyReachableOperands({ curr->ifTrue, curr->ifFalse, curr->condition }, curr->type);
   }
 
   void visitDrop(Drop* curr) {
diff --git a/test/passes/dce.txt b/test/passes/dce.txt
@@ -4,6 +4,8 @@
  (type $2 (func (result i32)))
  (type $3 (func (param i32) (result i32)))
  (type $4 (func (param i64 i64) (result i64)))
+ (type $5 (func (param f32 i64)))
+ (type $6 (func (param f32 i64) (result i32)))
  (global $x (mut i32) (i32.const 0))
  (table 1 1 anyfunc)
  (elem (i32.const 0) $call-me)
@@ -441,4 +443,48 @@
    (unreachable)
   )
  )
+ (func $replace-with-unreachable-affects-parent (type $5) (param $var$0 f32) (param $var$1 i64)
+  (block $top
+   (drop
+    (f32.load offset=4
+     (block (result i32)
+      (drop
+       (i64.const 0)
+      )
+      (if
+       (block $block (result i32)
+        (call $replace-with-unreachable-affects-parent
+         (f32.const 1)
+         (i64.const -15917430362925035)
+        )
+        (i32.const 1)
+       )
+       (unreachable)
+       (unreachable)
+      )
+     )
+    )
+   )
+   (unreachable)
+  )
+ )
+ (func $replace-block-changes-later-when-if-goes (type $1)
+  (block $top
+   (set_global $global$0
+    (i32.const 0)
+   )
+   (block $inner
+    (drop
+     (call $helper
+      (f32.const 1)
+      (i64.const -15917430362925035)
+     )
+    )
+    (unreachable)
+   )
+  )
+ )
+ (func $helper (type $6) (param $var$0 f32) (param $var$1 i64) (result i32)
+  (i32.const 0)
+ )
 )
diff --git a/test/passes/dce.wast b/test/passes/dce.wast
@@ -651,4 +651,60 @@
     )
   )
  )
+ (func $replace-with-unreachable-affects-parent (param $var$0 f32) (param $var$1 i64)
+  (block $top
+   (drop
+    (f32.load offset=4
+     (i64.ne
+      (i64.const 0)
+      (if (result i64)
+       (block (result i32)
+        (call $replace-with-unreachable-affects-parent
+         (f32.const 1)
+         (i64.const -15917430362925035)
+        )
+        (i32.const 1)
+       )
+       (unreachable)
+       (unreachable)
+      )
+     )
+    )
+   )
+   (nop) ;; this is not reachable due to the above code, so we replace it with unreachable. type should go to parent
+  )
+ )
+ (func $replace-block-changes-later-when-if-goes
+  (block $top ;; and so should this
+   (set_global $global$0
+    (i32.const 0)
+   )
+   (drop
+    (f32.load offset=4
+     (i64.ne
+      (block $inner (result i64) ;; this becomes unreachable
+       (drop
+        (call $helper
+         (f32.const 1)
+         (i64.const -15917430362925035)
+        )
+       )
+       (unreachable)
+      )
+      (i64.const 0)
+     )
+    )
+   )
+   (if
+    (i32.load16_s offset=22 align=1
+     (i32.const 0)
+    )
+    (br $top) ;; this keeps the block none after the inner block gets unreachable. but it will vanish into unreachable itself
+    (unreachable)
+   )
+  )
+ )
+ (func $helper (param $var$0 f32) (param $var$1 i64) (result i32)
+  (i32.const 0)
+ )
 )
