[DFAJumpThreading] Don't insert existing edge to DomTree while unfolding

[UsmanNadeem]

The edge StartBlock -> EndBlock already exists before unfolding.

The instructions for applyUpdates() say that you are supposed not
to insert an existing edge.

Fixes issues reported by @mikaelholmen in #162802

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Usman Nadeem (UsmanNadeem)

Changes

The edge StartBlock -> EndBlock already exists before unfolding.

The instructions for applyUpdates() say that you are supposed not
to insert an existing edge.

Fixes issues reported by @mikaelholmen in #162802

---

Full diff: https://github.com/llvm/llvm-project/pull/163296.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp (+1-2)
• (modified) llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-transform.ll (+58)

diff --git a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
index ff5f390d6fe18..59120f19aebcb 100644
--- a/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp
@@ -266,8 +266,7 @@ void DFAJumpThreading::unfold(DomTreeUpdater *DTU, LoopInfo *LI,
     if (!ProfcheckDisableMetadataFixes)
       BI->setMetadata(LLVMContext::MD_prof,
                       SI->getMetadata(LLVMContext::MD_prof));
-    DTU->applyUpdates({{DominatorTree::Insert, StartBlock, EndBlock},
-                       {DominatorTree::Insert, StartBlock, NewBlock}});
+    DTU->applyUpdates({{DominatorTree::Insert, StartBlock, NewBlock}});
   } else {
     BasicBlock *EndBlock = SIUse->getParent();
     BasicBlock *NewBlockT = BasicBlock::Create(
diff --git a/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-transform.ll b/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-transform.ll
index 092c854890462..f3a4010a53337 100644
--- a/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-transform.ll
+++ b/llvm/test/Transforms/DFAJumpThreading/dfa-jump-threading-transform.ll
@@ -445,9 +445,67 @@ bb2:                                              ; preds = %select.unfold
   unreachable
 }
 
+
+define i16 @DTU_update_crash() {
+; CHECK-LABEL: @DTU_update_crash(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY_SELECTBLOCK:%.*]]
+; CHECK:       for.body.selectblock:
+; CHECK-NEXT:    br i1 false, label [[SWITCHBLOCK_JT0:%.*]], label [[SEL_SI_UNFOLD_FALSE_JT0:%.*]]
+; CHECK:       sel.si.unfold.false:
+; CHECK-NEXT:    br label [[SWITCHBLOCK:%.*]]
+; CHECK:       sel.si.unfold.false.jt0:
+; CHECK-NEXT:    [[DOTSI_UNFOLD_PHI_JT0:%.*]] = phi i32 [ 0, [[FOR_BODY_SELECTBLOCK]] ]
+; CHECK-NEXT:    br label [[SWITCHBLOCK_JT0]]
+; CHECK:       switchblock:
+; CHECK-NEXT:    [[SWITCHBLOCK_PHI:%.*]] = phi i32 [ poison, [[SEL_SI_UNFOLD_FALSE:%.*]] ]
+; CHECK-NEXT:    [[P_24_ADDR_3:%.*]] = phi i32 [ 0, [[SEL_SI_UNFOLD_FALSE]] ]
+; CHECK-NEXT:    switch i32 [[SWITCHBLOCK_PHI]], label [[CLEANUP:%.*]] [
+; CHECK-NEXT:      i32 0, label [[FOR_INC:%.*]]
+; CHECK-NEXT:      i32 1, label [[CLEANUP]]
+; CHECK-NEXT:      i32 5, label [[FOR_BODY_SELECTBLOCK]]
+; CHECK-NEXT:    ]
+; CHECK:       switchblock.jt0:
+; CHECK-NEXT:    [[SWITCHBLOCK_PHI_JT0:%.*]] = phi i32 [ 0, [[FOR_BODY_SELECTBLOCK]] ], [ [[DOTSI_UNFOLD_PHI_JT0]], [[SEL_SI_UNFOLD_FALSE_JT0]] ]
+; CHECK-NEXT:    [[P_24_ADDR_3_JT0:%.*]] = phi i32 [ 0, [[FOR_BODY_SELECTBLOCK]] ], [ 0, [[SEL_SI_UNFOLD_FALSE_JT0]] ]
+; CHECK-NEXT:    br label [[FOR_INC]]
+; CHECK:       for.inc:
+; CHECK-NEXT:    br i1 false, label [[FOR_BODY_SELECTBLOCK]], label [[CLEANUP]]
+; CHECK:       cleanup:
+; CHECK-NEXT:    call void (...) @llvm.fake.use(i32 [[P_24_ADDR_3_JT0]])
+; CHECK-NEXT:    ret i16 0
+;
+entry:
+  br label %for.body.selectblock
+
+for.body.selectblock:                                        ; preds = %for.inc, %switchblock, %entry
+  %sel = select i1 false, i32 0, i32 0
+  br label %switchblock
+
+switchblock:                                       ; preds = %for.body.selectblock
+  %switchblock.phi = phi i32 [ %sel, %for.body.selectblock ]
+  %p_24.addr.3 = phi i32 [ 0, %for.body.selectblock ]
+  switch i32 %switchblock.phi, label %cleanup [
+  i32 0, label %for.inc
+  i32 1, label %cleanup
+  i32 5, label %for.body.selectblock
+  ]
+
+for.inc:                                       ; preds = %switchblock
+  br i1 false, label %for.body.selectblock, label %cleanup
+
+cleanup:                                       ; preds = %for.inc, %switchblock, %switchblock
+  call void (...) @llvm.fake.use(i32 %p_24.addr.3)
+  ret i16 0
+}
+
+declare void @llvm.fake.use(...)
+
 !0 = !{!"function_entry_count", i32 10}
 !1 = !{!"branch_weights", i32 3, i32 5}
 ;.
+; CHECK: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: readwrite) }
+;.
 ; CHECK: [[META0:![0-9]+]] = !{!"function_entry_count", i32 10}
 ; CHECK: [[PROF1]] = !{!"branch_weights", i32 3, i32 5}
 ;.

[mikaelholmen]

I've verified that this patch fixes the crashes I saw. Thanks!

[XChy]

We may also need to delete the edge StartBlock -> EndBlock and insert the edge StartBlock -> EndBlock here:

llvm-project/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp

Lines 308 to 310 in 489a921

	DTU->applyUpdates({{DominatorTree::Insert, NewBlockT, NewBlockF},
	{DominatorTree::Insert, NewBlockT, EndBlock},
	{DominatorTree::Insert, NewBlockF, EndBlock}});

[UsmanNadeem]

We may also need to delete the edge StartBlock -> EndBlock and insert the edge StartBlock -> EndBlock here:

llvm-project/llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp

Lines 308 to 310 in 489a921

	DTU->applyUpdates({{DominatorTree::Insert, NewBlockT, NewBlockF},
	{DominatorTree::Insert, NewBlockT, EndBlock},
	{DominatorTree::Insert, NewBlockF, EndBlock}});

We do this a few lines down:

    {{DominatorTree::Delete, StartBlock, EndBlock},
     {DominatorTree::Insert, StartBlock, NewBlockT}}

[XChy]

LGTM