Commit 181628b5 authored by Juneyoung Lee's avatar Juneyoung Lee
Browse files

[SimpleLoopUnswitch] Fix introduction of UB when hoisted condition may be undef or poison 

Summary:
Loop unswitch hoists branches on loop-invariant conditions. However, if this
condition is poison/undef and the branch wasn't originally reachable, loop
unswitch introduces UB (since the optimized code will branch on poison/undef and
the original one didn't)).
We fix this problem by freezing the condition to ensure we don't introduce UB.

We will now transform the following:
  while (...) {
    if (C) { A }
    else   { B }
  }

Into:
  C' = freeze(C)
  if (C') {
    while (...) { A }
  } else {
    while (...) { B }
  }

This patch fixes the root cause of the following bug reports (which use the old loop unswitch, but can be reproduced with minor changes in the code and -enable-nontrivial-unswitch):
- https://llvm.org/bugs/show_bug.cgi?id=27506
- https://llvm.org/bugs/show_bug.cgi?id=31652

Reviewers: reames, majnemer, chenli, sanjoy, hfinkel

Reviewed By: reames

Subscribers: hiraditya, jvesely, nhaehnle, filcab, regehr, trentxintong, nlopes, llvm-commits, mzolotukhin

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D29015
parent b083d7a3

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

+25 −4
Original line number Diff line number Diff line
@@ -26,7 +26,9 @@ 
#include "llvm/Analysis/LoopPass.h"

#include "llvm/Analysis/MemorySSA.h"

#include "llvm/Analysis/MemorySSAUpdater.h"

#include "llvm/Analysis/MustExecute.h"

#include "llvm/Analysis/Utils/Local.h"

#include "llvm/Analysis/ValueTracking.h"

#include "llvm/IR/BasicBlock.h"

#include "llvm/IR/Constant.h"

#include "llvm/IR/Constants.h"
@@ -180,11 +182,14 @@ static void buildPartialUnswitchConditionalBranch(BasicBlock &BB, 
                                                  ArrayRef<Value *> Invariants,

                                                  bool Direction,

                                                  BasicBlock &UnswitchedSucc,

                                                  BasicBlock &NormalSucc) {

                                                  BasicBlock &NormalSucc,

                                                  bool insertFreeze) {

  IRBuilder<> IRB(&BB);



  Value *Cond = Direction ? IRB.CreateOr(Invariants) :

    IRB.CreateAnd(Invariants);

  if (insertFreeze)

    Cond = IRB.CreateFreeze(Cond, Cond->getName() + ".fr");

  IRB.CreateCondBr(Cond, Direction ? &UnswitchedSucc : &NormalSucc,

                   Direction ? &NormalSucc : &UnswitchedSucc);

}
@@ -498,7 +503,7 @@ static bool unswitchTrivialBranch(Loop &L, BranchInst &BI, DominatorTree &DT, 
                 Instruction::And &&

             "Must have an `and` of `i1`s for the condition!");

    buildPartialUnswitchConditionalBranch(*OldPH, Invariants, ExitDirection,

                                          *UnswitchedBB, *NewPH);

                                          *UnswitchedBB, *NewPH, false);

  }



  // Update the dominator tree with the added edge.
@@ -2009,6 +2014,10 @@ static void unswitchNontrivialInvariants( 
      SE->forgetTopmostLoop(&L);

  }



  ICFLoopSafetyInfo SafetyInfo(&DT);

  SafetyInfo.computeLoopSafetyInfo(&L);

  bool insertFreeze = !SafetyInfo.isGuaranteedToExecute(TI, &DT, &L);



  // If the edge from this terminator to a successor dominates that successor,

  // store a map from each block in its dominator subtree to it. This lets us

  // tell when cloning for a particular successor if a block is dominated by
@@ -2066,6 +2075,12 @@ static void unswitchNontrivialInvariants( 
      BasicBlock *ClonedPH = ClonedPHs.begin()->second;

      BI->setSuccessor(ClonedSucc, ClonedPH);

      BI->setSuccessor(1 - ClonedSucc, LoopPH);

      if (insertFreeze) {

        auto Cond = BI->getCondition();

        if (!isGuaranteedNotToBeUndefOrPoison(Cond))

          BI->setCondition(new FreezeInst(Cond, Cond->getName() + ".fr", BI));

      }



      DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});

    } else {

      assert(SI && "Must either be a branch or switch!");
@@ -2080,6 +2095,12 @@ static void unswitchNontrivialInvariants( 
        else

          Case.setSuccessor(ClonedPHs.find(Case.getCaseSuccessor())->second);



      if (insertFreeze) {

        auto Cond = SI->getCondition();

        if (!isGuaranteedNotToBeUndefOrPoison(Cond))

          SI->setCondition(new FreezeInst(Cond, Cond->getName() + ".fr", SI));

      }



      // We need to use the set to populate domtree updates as even when there

      // are multiple cases pointing at the same successor we only want to

      // remove and insert one edge in the domtree.
@@ -2156,7 +2177,7 @@ static void unswitchNontrivialInvariants( 
    // When doing a partial unswitch, we have to do a bit more work to build up

    // the branch in the split block.

    buildPartialUnswitchConditionalBranch(*SplitBB, Invariants, Direction,

                                          *ClonedPH, *LoopPH);

                                          *ClonedPH, *LoopPH, insertFreeze);

    DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});



    if (MSSAU) {

llvm/test/Transforms/SimpleLoopUnswitch/exponential-nontrivial-unswitch-nested.ll

+7 −1
Original line number Diff line number Diff line
@@ -85,8 +85,14 @@ 


declare void @bar()



define void @loop_nested3_conds5(i32* %addr, i1 %c1, i1 %c2, i1 %c3, i1 %c4, i1 %c5) {

define void @loop_nested3_conds5(i32* %addr, i1 %c1i, i1 %c2i, i1 %c3i, i1 %c4i, i1 %c5i) {

entry:

  ; c1 ~ c5 are guaranteed to be never undef or poison.

  %c1 = freeze i1 %c1i

  %c2 = freeze i1 %c2i

  %c3 = freeze i1 %c3i

  %c4 = freeze i1 %c4i

  %c5 = freeze i1 %c5i

  %addr1 = getelementptr i32, i32* %addr, i64 0

  %addr2 = getelementptr i32, i32* %addr, i64 1

  %addr3 = getelementptr i32, i32* %addr, i64 2

llvm/test/Transforms/SimpleLoopUnswitch/exponential-nontrivial-unswitch-nested2.ll

+7 −1
Original line number Diff line number Diff line
@@ -97,8 +97,14 @@ 


declare void @bar()



define void @loop_nested3_conds5(i32* %addr, i1 %c1, i1 %c2, i1 %c3, i1 %c4, i1 %c5) {

define void @loop_nested3_conds5(i32* %addr, i1 %c1i, i1 %c2i, i1 %c3i, i1 %c4i, i1 %c5i) {

entry:

  ; c1 ~ c5 are guaranteed to be never undef or poison.

  %c1 = freeze i1 %c1i

  %c2 = freeze i1 %c2i

  %c3 = freeze i1 %c3i

  %c4 = freeze i1 %c4i

  %c5 = freeze i1 %c5i

  %addr1 = getelementptr i32, i32* %addr, i64 0

  %addr2 = getelementptr i32, i32* %addr, i64 1

  %addr3 = getelementptr i32, i32* %addr, i64 2

llvm/test/Transforms/SimpleLoopUnswitch/exponential-switch-unswitch.ll

+4 −1
Original line number Diff line number Diff line
@@ -86,8 +86,11 @@ 
; LOOP-MAX-COUNT-111:     Loop at depth 2 containing:

; LOOP-MAX-NOT: Loop at depth 2 containing:



define i32 @loop_switch(i32* %addr, i32 %c1, i32 %c2) {

define i32 @loop_switch(i32* %addr, i32 %c1i, i32 %c2i) {

entry:

  ; c1, c2 are guaranteed to be never undef or poison.

  %c1 = freeze i32 %c1i

  %c2 = freeze i32 %c2i

  %addr1 = getelementptr i32, i32* %addr, i64 0

  %addr2 = getelementptr i32, i32* %addr, i64 1

  %check0 = icmp eq i32 %c2, 0

llvm/test/Transforms/SimpleLoopUnswitch/guards.ll

+2 −1
Original line number Diff line number Diff line
@@ -81,7 +81,8 @@ exit: 
define void @test_conditional_guards(i1 %cond, i32 %N) {

; CHECK-LABEL: @test_conditional_guards(

; CHECK-NEXT:  entry:

; CHECK-NEXT:    br i1 [[COND:%.*]], label [[ENTRY_SPLIT_US:%.*]], label [[ENTRY_SPLIT:%.*]]

; CHECK-NEXT:    [[COND_FR:%.*]] = freeze i1 [[COND:%.*]]

; CHECK-NEXT:    br i1 [[COND_FR]], label [[ENTRY_SPLIT_US:%.*]], label [[ENTRY_SPLIT:%.*]]

; CHECK:       entry.split.us:

; CHECK-NEXT:    br label [[LOOP_US:%.*]]

; CHECK:       loop.us: