[PATCH] D119760: [DSE] Fall back to CFG scan for unreachable terminators.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 408998.
fhahn marked an inline comment as done.
fhahn added a comment.

In D119760#3322493 <https://reviews.llvm.org/D119760#3322493>, @nikic wrote:

> Please rebase over https://github.com/llvm/llvm-project/commit/2460a2ce47873109e9b300f65e1bd2f4c4b86bfb.

Done,thanks!

In D119760#3321039 <https://reviews.llvm.org/D119760#3321039>, @efriedma wrote:

> As long as you've thought about it, I guess...
>
> I'm a little confused by the different traversals, though.  Have you considered just treating "ret" as reading memory, instead of analyzing "ret" using a different codepath from unwinding?

Initially one approach was to model reads at function exits through a special intrinsic (D73763 <https://reviews.llvm.org/D73763>), but in the end we went with the current approach of the extra checks.

I put up a new patch that introduces dummy loads just before exits to see how it would look in terms of compile-time, and it looks mostly neutral, with slightly more stores removed: D119879 <https://reviews.llvm.org/D119879>. One concern with the original patch was the use of a new intrinsic. This patch just adds dummy loads, not sure if there's a better way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119760/new/

https://reviews.llvm.org/D119760

Files:
  llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
  llvm/test/Transforms/DeadStoreElimination/multiblock-unreachable.ll


Index: llvm/test/Transforms/DeadStoreElimination/multiblock-unreachable.ll
===================================================================
--- llvm/test/Transforms/DeadStoreElimination/multiblock-unreachable.ll
+++ llvm/test/Transforms/DeadStoreElimination/multiblock-unreachable.ll
@@ -63,12 +63,11 @@
 define void @unreachable_exit_with_no_call(i64* noalias %ptr, i1 %c.1) {
 ; CHECK-LABEL: @unreachable_exit_with_no_call(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    store i64 1, i64* [[PTR:%.*]], align 8
 ; CHECK-NEXT:    br i1 [[C_1:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.then:
 ; CHECK-NEXT:    unreachable
 ; CHECK:       if.end:
-; CHECK-NEXT:    store i64 0, i64* [[PTR]], align 8
+; CHECK-NEXT:    store i64 0, i64* [[PTR:%.*]], align 8
 ; CHECK-NEXT:    ret void
 ;
 entry:
@@ -87,13 +86,12 @@
 define void @unreachable_exit_with_nounwind_call_pr53800(i64* noalias %ptr, i1 %c.1) {
 ; CHECK-LABEL: @unreachable_exit_with_nounwind_call_pr53800(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    store i64 1, i64* [[PTR:%.*]], align 8
 ; CHECK-NEXT:    br i1 [[C_1:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.then:
 ; CHECK-NEXT:    tail call void @exit() #[[ATTR0:[0-9]+]]
 ; CHECK-NEXT:    unreachable
 ; CHECK:       if.end:
-; CHECK-NEXT:    store i64 0, i64* [[PTR]], align 8
+; CHECK-NEXT:    store i64 0, i64* [[PTR:%.*]], align 8
 ; CHECK-NEXT:    ret void
 ;
 entry:
Index: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -770,6 +770,10 @@
   /// Keep track of instructions (partly) overlapping with killing MemoryDefs per
   /// basic block.
   MapVector<BasicBlock *, InstOverlapIntervalsTy> IOLs;
+  // Check if there are root nodes that are terminated by UnreachableInst.
+  // Those roots pessimize post-dominance queries. If there are such roots,
+  // fall back to CFG scan starting from all non-unreachable roots.
+  bool AnyUnreachableExit;
 
   // Class contains self-reference, make sure it's not copied/moved.
   DSEState(const DSEState &) = delete;
@@ -805,6 +809,10 @@
 
     // Collect whether there is any irreducible control flow in the function.
     ContainsIrreducibleLoops = mayContainIrreducibleControl(F, &LI);
+
+    AnyUnreachableExit = any_of(PDT.roots(), [](const BasicBlock *E) {
+      return isa<UnreachableInst>(E->getTerminator());
+    });
   }
 
   /// Return 'OW_Complete' if a store to the 'KillingLoc' location (by \p
@@ -1511,22 +1519,32 @@
       // If the common post-dominator does not post-dominate MaybeDeadAccess,
       // there is a path from MaybeDeadAccess to an exit not going through a
       // killing block.
-      if (!PDT.dominates(CommonPred, MaybeDeadAccess->getBlock()))
+      bool AllPathsToExitThroughCommonPred =
+          PDT.dominates(CommonPred, MaybeDeadAccess->getBlock());
+      if (!AllPathsToExitThroughCommonPred && !AnyUnreachableExit)
         return None;
 
       // If CommonPred itself is in the set of killing blocks, we're done.
-      if (KillingBlocks.count(CommonPred))
+      if (AllPathsToExitThroughCommonPred && KillingBlocks.count(CommonPred))
         return {MaybeDeadAccess};
 
       SetVector<BasicBlock *> WorkList;
 
+      if (!AllPathsToExitThroughCommonPred) {
+        // Fall back to CFG scan starting at all non-unreachable roots if not
+        // all paths to the exit go through CommonPred.
+        CommonPred = nullptr;
+      }
+
       // If CommonPred is null, there are multiple exits from the function.
       // They all have to be added to the worklist.
       if (CommonPred)
         WorkList.insert(CommonPred);
       else
-        for (BasicBlock *R : PDT.roots())
-          WorkList.insert(R);
+        for (BasicBlock *R : PDT.roots()) {
+          if (!isa<UnreachableInst>(R->getTerminator()))
+            WorkList.insert(R);
+        }
 
       NumCFGTries++;
       // Check if all paths starting from an exit node go through one of the


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D119760.408998.patch
Type: text/x-patch
Size: 4118 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220215/11f07591/attachment.bin>