[PATCH] D32715: shrink-wrap: fix shrink-wrapping for no-return paths

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg created this revision.

Shrink-wrapping uses post-dominators to find a restore point that post-dominates all the uses of CSR / stack.

The way dominator trees are modeled in LLVM today is that unreachable blocks are not present in a generic dominator tree. I am actually guessing this from some comments: include/llvm/Support/GenericDomTree.h:423 <https://github.com/llvm-mirror/llvm/blob/74d2cb54e4d2d008fb0428510924ba7545a1c912/include/llvm/Support/GenericDomTree.h#L423>, 
So, an unreachable node is dominated by anything: include/llvm/Support/GenericDomTree.h:467 <https://github.com/llvm-mirror/llvm/blob/74d2cb54e4d2d008fb0428510924ba7545a1c912/include/llvm/Support/GenericDomTree.h#L467>.
There seems to be an explanation that an unreachable block may or may not appear in the dominator tree: include/llvm/IR/Dominators.h:103 <https://github.com/llvm-mirror/llvm/blob/74d2cb54e4d2d008fb0428510924ba7545a1c912/include/llvm/IR/Dominators.h#L103>:

  /// A forward unreachable block may appear in the dominator tree, or it may
  /// not.  If it does, dominance queries will return results as if all reachable
  /// blocks dominate it.  When asking for a Node corresponding to a potentially
  /// unreachable block, calling code must handle the case where the block was
  /// unreachable and the result of getNode() is nullptr.
  ///
  /// Generally, a block known to be unreachable when the dominator tree is
  /// constructed will not be in the tree.  One which becomes unreachable after
  /// the dominator tree is initially constructed may still exist in the tree,
  /// even if the tree is properly updated. Calling code should not rely on the
  /// preceding statements; this is stated only to assist human understanding.

From that, I assume the MachineDominators might work in the same way.

Since for post-dominators, a no-return block is considered "unreachable", calling `findNearestCommonDominator` on an unreachable node A and a non-unreachable node B, will return B, which can be false.

I suggest that if we find such a node, we bail out since there is no good restore point.


https://reviews.llvm.org/D32715

Files:
  lib/CodeGen/ShrinkWrap.cpp
  test/CodeGen/X86/x86-shrink-wrapping.ll


Index: test/CodeGen/X86/x86-shrink-wrapping.ll
===================================================================
--- test/CodeGen/X86/x86-shrink-wrapping.ll
+++ test/CodeGen/X86/x86-shrink-wrapping.ll
@@ -270,8 +270,6 @@
   ret i32 %sum.1
 }
 
-declare void @somethingElse(...)
-
 ; Check with a more complex case that we do not have restore within the loop and
 ; save outside.
 ; CHECK-LABEL: loopInfoRestoreOutsideLoop:
@@ -982,3 +980,54 @@
 }
 
 attributes #4 = { "no-frame-pointer-elim"="true" }
+
+ at x = external global i32, align 4
+ at y = external global i32, align 4
+
+; The post-dominator tree does not include the branch containing the infinite
+; loop, which can occur into a misplacement of the restore block, if we're
+; looking for the nearest common post-dominator of an "unreachable" block.
+
+; CHECK-LABEL: infiniteLoopNoSuccessor:
+; CHECK: ## BB#0:
+; Make sure the prologue happens in the entry block.
+; CHECK-NEXT: pushq %rbp
+; ...
+; Make sure we don't shrink-wrap.
+; CHECK: ## BB#1
+; CHECK-NOT: pushq %rbp
+; ...
+; Make sure the epilogue happens in the exit block.
+; CHECK: ## BB#5
+; CHECK: popq %rbp
+; CHECK-NEXT: retq
+define void @infiniteLoopNoSuccessor() #5 {
+  %1 = load i32, i32* @x, align 4
+  %2 = icmp ne i32 %1, 0
+  br i1 %2, label %3, label %4
+
+; <label>:3:
+  store i32 0, i32* @x, align 4
+  br label %4
+
+; <label>:4:
+  call void (...) @somethingElse()
+  %5 = load i32, i32* @y, align 4
+  %6 = icmp ne i32 %5, 0
+  br i1 %6, label %10, label %7
+
+; <label>:7:
+  %8 = call i32 (...) @something()
+  br label %9
+
+; <label>:9:
+  call void (...) @somethingElse()
+  br label %9
+
+; <label>:10:
+  ret void
+}
+
+declare void @somethingElse(...)
+
+attributes #5 = { nounwind  "no-frame-pointer-elim-non-leaf" }
Index: lib/CodeGen/ShrinkWrap.cpp
===================================================================
--- lib/CodeGen/ShrinkWrap.cpp
+++ lib/CodeGen/ShrinkWrap.cpp
@@ -282,8 +282,14 @@
 
   if (!Restore)
     Restore = &MBB;
-  else
+  else if (MPDT->getNode(&MBB)) // If the block is not in the post dom tree, it
+                                // means the block never returns. If that's the
+                                // case, we don't want to call
+                                // `findNearestCommonDominator`, which will
+                                // return `Restore`.
     Restore = MPDT->findNearestCommonDominator(Restore, &MBB);
+  else
+    Restore = nullptr;
 
   // Make sure we would be able to insert the restore code before the
   // terminator.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D32715.97330.patch
Type: text/x-patch
Size: 2546 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170501/089b04f8/attachment.bin>