[llvm] 45bd8d9 - [SimpleLoopUnswitch] Don't unswitch constant conditions

[Daniil Suchkov via llvm-commits]

Author: Daniil Suchkov
Date: 2021-10-01T21:30:54Z
New Revision: 45bd8d947780811aa4a6ba7e6b42e3a2dce3e6bd

URL: https://github.com/llvm/llvm-project/commit/45bd8d947780811aa4a6ba7e6b42e3a2dce3e6bd
DIFF: https://github.com/llvm/llvm-project/commit/45bd8d947780811aa4a6ba7e6b42e3a2dce3e6bd.diff

LOG: [SimpleLoopUnswitch] Don't unswitch constant conditions

Added an additional check for constants after simplification of
"select _, true, false" pattern. We need to prevent attempts to unswitch constant
conditions for two reasons:
a) Doing that doesn't make any sense, in the best case it will just burn
some compile time.
b) SimpleLoopUnswitch isn't designed to unswitch constant conditions
(due to (a)), so attempting that can cause miscompiles. The attached
testcase is an example of such miscompile.

Also added an assertion that'll make sure we aren't trying to replace
constants, so it will help us prevent such bugs in future. The assertion
from D110751 is another layer of protection against such cases.

Reviewed By: aeubanks
Differential Revision: https://reviews.llvm.org/D110752

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
    llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index 4042353ee52c8..eb068fae492a1 100644
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -2370,7 +2370,9 @@ static void unswitchNontrivialInvariants(
     ConstantInt *ContinueReplacement =
         Direction ? ConstantInt::getFalse(BI->getContext())
                   : ConstantInt::getTrue(BI->getContext());
-    for (Value *Invariant : Invariants)
+    for (Value *Invariant : Invariants) {
+      assert(!isa<Constant>(Invariant) &&
+             "Should not be replacing constant values!");
       // Use make_early_inc_range here as set invalidates the iterator.
       for (Use &U : llvm::make_early_inc_range(Invariant->uses())) {
         Instruction *UserI = dyn_cast<Instruction>(U.getUser());
@@ -2385,6 +2387,7 @@ static void unswitchNontrivialInvariants(
                  DT.dominates(ClonedPH, UserI->getParent()))
           U.set(UnswitchedReplacement);
       }
+    }
   }
 
   // We can change which blocks are exit blocks of all the cloned sibling
@@ -2727,6 +2730,9 @@ static bool unswitchBestCondition(
       Cond = CondNext;
     BI->setCondition(Cond);
 
+    if (isa<Constant>(Cond))
+      continue;
+
     if (L.isLoopInvariant(BI->getCondition())) {
       UnswitchCandidates.push_back({BI, {BI->getCondition()}});
       continue;

diff  --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
index 2f9d25c7307e1..4097d3523ab30 100644
--- a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-invariant-select-bug.ll
@@ -1,31 +1,22 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -passes='simple-loop-unswitch<nontrivial>' -S < %s | FileCheck %s
 
-; FIXME: We should not replace `true` with `false` here!
+; If we try to replace uses of `true` outside of `@foo`, we'll see it here.
 define i1 @bar() {
 ; CHECK-LABEL: @bar(
-; CHECK-NEXT:    ret i1 false
+; CHECK-NEXT:    ret i1 true
 ;
   ret i1 true
 }
 
-; FIXME: We shouldn't unswitch this loop!
+; We shouldn't unswitch this loop.
 define void @foo() {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br i1 true, label [[ENTRY_SPLIT_US:%.*]], label [[ENTRY_SPLIT:%.*]]
-; CHECK:       entry.split.us:
-; CHECK-NEXT:    br label [[HEADER_US:%.*]]
-; CHECK:       header.us:
-; CHECK-NEXT:    [[VAL_US:%.*]] = select i1 true, i1 true, i1 false
-; CHECK-NEXT:    br label [[EXIT_SPLIT_US:%.*]]
-; CHECK:       exit.split.us:
-; CHECK-NEXT:    br label [[EXIT:%.*]]
-; CHECK:       entry.split:
 ; CHECK-NEXT:    br label [[HEADER:%.*]]
 ; CHECK:       header:
-; CHECK-NEXT:    [[VAL:%.*]] = select i1 false, i1 false, i1 false
-; CHECK-NEXT:    br label [[HEADER]]
+; CHECK-NEXT:    [[VAL:%.*]] = select i1 true, i1 true, i1 false
+; CHECK-NEXT:    br i1 true, label [[EXIT:%.*]], label [[HEADER]]
 ; CHECK:       exit:
 ; CHECK-NEXT:    ret void
 ;