[GuardWidening] Fix widening possibility check

[aleks-tmb]

In the 0e0ff85 was introduced inconsistency between condition widening and checking if it's possible to widen. We check the possibility to hoist checks parsed from the condition, but hoist entire condition.

This patch returns testing that a condition can be hoisted rather than the checks parsed from that condition.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Changes

In the 0e0ff85 was introduced inconsistency between condition widening and checking if it's possible to widen. We check the possibility to hoist checks parsed from the condition, but hoist entire condition.

This patch returns testing that a condition can be hoisted rather than the checks parsed from that condition.

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/GuardWidening.cpp (+9-5)
• (added) llvm/test/Transforms/GuardWidening/hoist-checks.ll (+45)

diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index 701cd1b90950d55..fb1db11e85a0b8f 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -184,6 +184,7 @@ class GuardWideningImpl {
   /// Compute the score for widening the condition in \p DominatedInstr
   /// into \p WideningPoint.
   WideningScore computeWideningScore(Instruction *DominatedInstr,
+                                     Instruction *ToWiden,
                                      Instruction *WideningPoint,
                                      SmallVectorImpl &ChecksToHoist,
                                      SmallVectorImpl &ChecksToWiden);
@@ -423,8 +424,8 @@ bool GuardWideningImpl::eliminateInstrViaWidening(
         continue;
       SmallVector CandidateChecks;
       parseWidenableGuard(Candidate, CandidateChecks);
-      auto Score = computeWideningScore(Instr, WideningPoint, ChecksToHoist,
-                                        CandidateChecks);
+      auto Score = computeWideningScore(Instr, Candidate, WideningPoint,
+                                        ChecksToHoist, CandidateChecks);
       LLVM_DEBUG(dbgs() << "Score between " << *Instr << " and " << *Candidate
                         << " is " << scoreTypeToString(Score) << "\n");
       if (Score > BestScoreSoFar) {
@@ -456,8 +457,8 @@ bool GuardWideningImpl::eliminateInstrViaWidening(
 }
 
 GuardWideningImpl::WideningScore GuardWideningImpl::computeWideningScore(
-    Instruction *DominatedInstr, Instruction *WideningPoint,
-    SmallVectorImpl &ChecksToHoist,
+    Instruction *DominatedInstr, Instruction *ToWiden,
+    Instruction *WideningPoint, SmallVectorImpl &ChecksToHoist,
     SmallVectorImpl &ChecksToWiden) {
   Loop *DominatedInstrLoop = LI.getLoopFor(DominatedInstr->getParent());
   Loop *DominatingGuardLoop = LI.getLoopFor(WideningPoint->getParent());
@@ -475,7 +476,10 @@ GuardWideningImpl::WideningScore GuardWideningImpl::computeWideningScore(
 
   if (!canBeHoistedTo(ChecksToHoist, WideningPoint))
     return WS_IllegalOrNegative;
-  if (!canBeHoistedTo(ChecksToWiden, WideningPoint))
+  // Further in the GuardWideningImpl::hoistChecks the entire condition might be
+  // widened, not the parsed list of checks. So we need to check the possibility
+  // of that condition hoisting.
+  if (!canBeHoistedTo(getCondition(ToWiden), WideningPoint))
     return WS_IllegalOrNegative;
 
   // If the guard was conditional executed, it may never be reached
diff --git a/llvm/test/Transforms/GuardWidening/hoist-checks.ll b/llvm/test/Transforms/GuardWidening/hoist-checks.ll
new file mode 100644
index 000000000000000..7220b3d7a4a99cd
--- /dev/null
+++ b/llvm/test/Transforms/GuardWidening/hoist-checks.ll
@@ -0,0 +1,45 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -passes=guard-widening < %s | FileCheck %s
+
+declare void @llvm.experimental.deoptimize.isVoid(...)
+declare i1 @llvm.experimental.widenable.condition()
+
+; In the current scheme we widen `br i1 %and1`, so we try to build
+; `and i1 poison, %and1` using `%call0` as insertion point.
+; That's not possible, so widening will not occur in the case.
+; TODO: Widen `%call0` in the test case, not a branch.
+define void @test() {
+; CHECK-LABEL: @test(
+; CHECK-NEXT:  bb0:
+; CHECK-NEXT:    [[CALL0:%.*]] = call i1 @llvm.experimental.widenable.condition()
+; CHECK-NEXT:    [[AND0:%.*]] = and i1 false, [[CALL0]]
+; CHECK-NEXT:    [[AND1:%.*]] = and i1 false, [[AND0]]
+; CHECK-NEXT:    br i1 [[AND1]], label [[BB1:%.*]], label [[DEOPT:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[CALL1:%.*]] = call i1 @llvm.experimental.widenable.condition()
+; CHECK-NEXT:    [[AND2:%.*]] = and i1 poison, [[CALL1]]
+; CHECK-NEXT:    br i1 [[AND2]], label [[UNREACH:%.*]], label [[DEOPT]]
+; CHECK:       unreach:
+; CHECK-NEXT:    unreachable
+; CHECK:       deopt:
+; CHECK-NEXT:    call void (...) @llvm.experimental.deoptimize.isVoid(i32 0) [ "deopt"(i32 0) ]
+; CHECK-NEXT:    ret void
+;
+bb0:
+  %call0 = call i1 @llvm.experimental.widenable.condition()
+  %and0 = and i1 false, %call0
+  %and1 = and i1 false, %and0
+  br i1 %and1, label %bb1, label %deopt
+
+bb1:
+  %call1 = call i1 @llvm.experimental.widenable.condition()
+  %and2 = and i1 poison, %call1
+  br i1 %and2, label %unreach, label %deopt
+
+unreach:
+  unreachable
+
+deopt:
+  call void (...) @llvm.experimental.deoptimize.isVoid(i32 0) [ "deopt"(i32 0) ]
+  ret void
+}

[annamthomas]

LGTM.