[llvm] Revert "[StructurizeCFG] Refactor insertConditions. NFC. (#115476)" (PR #136370)

[Shilei Tian via llvm-commits]

https://github.com/shiltian created https://github.com/llvm/llvm-project/pull/136370

This reverts commit `231e63d8162a1c78a973c6e546bea39d04fefd67` because it was
intended to be NFC, but it actually introduces a semantic change. The behavior
is not equivalent to the original code.

Specifically, the change attempts to make the special case of `Parent` being the
only predecessor more explicit by isolating it, and then asserts in the `else`
branch that `BB` can't be `Parent`. However, it's still possible for
`Preds.size() > 1`, in which case `BB` can be `Parent`, causing the assertion to
trigger.

Eventually, this introduced a regression (though the related test case wasn't in
the repo at the time). To fix it properly, we'd have to reintroduce logic in the
`else` block to handle the case where `BB` is `Parent`, which counters the
intent of the original refactor. Because of that, I think it's cleaner and more
reliable to simply revert the commit.

Fixes #126534.

>From a644088a753cf893e57eb817b5fdc7de79b72154 Mon Sep 17 00:00:00 2001
From: Shilei Tian <i at tianshilei.me>
Date: Fri, 18 Apr 2025 17:25:45 -0400
Subject: [PATCH] Revert "[StructurizeCFG] Refactor insertConditions. NFC.
 (#115476)"

This reverts commit `231e63d8162a1c78a973c6e546bea39d04fefd67` because it was
intended to be NFC, but it actually introduces a semantic change. The behavior
is not equivalent to the original code.

Specifically, the change attempts to make the special case of `Parent` being the
only predecessor more explicit by isolating it, and then asserts in the `else`
branch that `BB` can't be `Parent`. However, it's still possible for
`Preds.size() > 1`, in which case `BB` can be `Parent`, causing the assertion to
trigger.

Eventually, this introduced a regression (though the related test case wasn't in
the repo at the time). To fix it properly, we'd have to reintroduce logic in the
`else` block to handle the case where `BB` is `Parent`, which counters the
intent of the original refactor. Because of that, I think it's cleaner and more
reliable to simply revert the commit.

Fixes #126534.
---
 llvm/lib/Transforms/Scalar/StructurizeCFG.cpp | 31 ++++++++++---------
 .../simple-structurizecfg-crash.ll            | 16 +++++++---
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
index 00c4fcc76e791..eb22b50532695 100644
--- a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
+++ b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
@@ -612,25 +612,28 @@ void StructurizeCFG::insertConditions(bool Loops) {
     BasicBlock *SuccTrue = Term->getSuccessor(0);
     BasicBlock *SuccFalse = Term->getSuccessor(1);
 
-    BBPredicates &Preds = Loops ? LoopPreds[SuccFalse] : Predicates[SuccTrue];
+    PhiInserter.Initialize(Boolean, "");
+    PhiInserter.AddAvailableValue(Loops ? SuccFalse : Parent, Default);
 
-    if (Preds.size() == 1 && Preds.begin()->first == Parent) {
-      auto &PI = Preds.begin()->second;
-      Term->setCondition(PI.Pred);
-      CondBranchWeights::setMetadata(*Term, PI.Weights);
-    } else {
-      PhiInserter.Initialize(Boolean, "");
-      PhiInserter.AddAvailableValue(Loops ? SuccFalse : Parent, Default);
+    BBPredicates &Preds = Loops ? LoopPreds[SuccFalse] : Predicates[SuccTrue];
 
-      NearestCommonDominator Dominator(DT);
-      Dominator.addBlock(Parent);
+    NearestCommonDominator Dominator(DT);
+    Dominator.addBlock(Parent);
 
-      for (auto [BB, PI] : Preds) {
-        assert(BB != Parent);
-        PhiInserter.AddAvailableValue(BB, PI.Pred);
-        Dominator.addAndRememberBlock(BB);
+    PredInfo ParentInfo{nullptr, std::nullopt};
+    for (auto [BB, PI] : Preds) {
+      if (BB == Parent) {
+        ParentInfo = PI;
+        break;
       }
+      PhiInserter.AddAvailableValue(BB, PI.Pred);
+      Dominator.addAndRememberBlock(BB);
+    }
 
+    if (ParentInfo.Pred) {
+      Term->setCondition(ParentInfo.Pred);
+      CondBranchWeights::setMetadata(*Term, ParentInfo.Weights);
+    } else {
       if (!Dominator.resultIsRememberedBlock())
         PhiInserter.AddAvailableValue(Dominator.result(), Default);
 
diff --git a/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll b/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll
index 6edc0b883f51e..691f43bdcf948 100644
--- a/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll
+++ b/llvm/test/Transforms/StructurizeCFG/simple-structurizecfg-crash.ll
@@ -1,11 +1,19 @@
-; RUN: opt -S -passes=structurizecfg %s -o -
-; REQUIRES: asserts
-; XFAIL: *
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=structurizecfg %s -o - | FileCheck %s
 
 ; Issue tracking: https://github.com/llvm/llvm-project/issues/126534.
-; FIXME: This test is expected to crash. Generate checklines after the crash is fixed.
 
 define void @foo() {
+; CHECK-LABEL: define void @foo() {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    br label %[[COND_FALSE:.*]]
+; CHECK:       [[COND_TRUE:.*]]:
+; CHECK-NEXT:    br label %[[COND_END:.*]]
+; CHECK:       [[COND_FALSE]]:
+; CHECK-NEXT:    br i1 false, label %[[COND_TRUE]], label %[[COND_END]]
+; CHECK:       [[COND_END]]:
+; CHECK-NEXT:    ret void
+;
 entry:
   br i1 false, label %cond.true, label %cond.false