Handling null AssumptionCache in simplifyCFG

[rcorcs]

Summary

AssumptionCache can be null in SimplifyCFGOptions. However, FoldCondBranchOnPHI() was not properly handling that when passing a null AssumptionCache to simplifyCFG.

[lebedev.ri]

Test?

[rcorcs]

Added test case that exercises this issue.

[rcorcs]

Simplified test case.

[fhahn]

LGTM with a few small suggestions. Please wait a bit with committing in case there are additional comments.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

2272

While you are at it, it might be worth simplifying the condition to something like if (AC && match(N, m_Intrinsic<IntrinsicInst::assume>())

llvm/unittests/Transforms/Utils/LocalTest.cpp

989

might be worth stating that this is just to get a pointer to %test.bb. You could also just check the BB name, which is slightly more direct. Also, it would be good to assert that we found TestBB.

996

nit: you could just use EXPECT_TRUE directly, I do not think the variable adds much.

[spatel]

LGTM with the changes suggested.

[rcorcs]

Addressed comments.

Thanks everyone for the review.

Can someone please commit this patch for me?

[fhahn]

Thanks, I'll land the patch

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

2271

This is not required the using match as below. I'll update it before committing

[thakis]

Looks like this breaks tests: http://45.33.8.238/linux/5220/step_12.txt

[xbolva00]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

2273

if (AC && N && match(N, m_Intrinsic<Intrinsic::assume>()))

?

[fhahn]

Should be fixed by c25de56905b104fb2b90559ce5863f4fec93a974

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

for (BasicBlock::iterator BBI = BB->begin(); &*BBI != BI; ++BBI) {
         if (!BBI->use_empty())
           TranslateMap[&*BBI] = N;
       }
       // Insert the new instruction into its new home.
       if (N)
         EdgeBB->getInstList().insert(InsertPt, N);
 
       // Register the new instruction with the assumption cache if necessary.
       if (auto *II = dyn_cast_or_null<IntrinsicInst>(N))
-        if (II->getIntrinsicID() == Intrinsic::assume)
+        if (AC && II->getIntrinsicID() == Intrinsic::assume)
           AC->registerAssumption(II);
     }
 
     // Loop over all of the edges from PredBB to BB, changing them to branch
     // to EdgeBB instead.
     Instruction *PredBBTI = PredBB->getTerminator();
     for (unsigned i = 0, e = PredBBTI->getNumSuccessors(); i != e; ++i)
       if (PredBBTI->getSuccessor(i) == BB) {
         BB->removePredecessor(PredBB);

llvm/unittests/Transforms/Utils/LocalTest.cpp

 //===- Local.cpp - Unit tests for Local -----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
 #include "llvm/Analysis/PostDominators.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Verifier.h"
auto checkRUBlocksRetVal = [&](Function &F, DominatorTree *DT) {
     DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Lazy);
     EXPECT_TRUE(removeUnreachableBlocks(F, &DTU));
     EXPECT_FALSE(removeUnreachableBlocks(F, &DTU));
     EXPECT_TRUE(DTU.getDomTree().verify());
   };
 
   runWithDomTree(*M, "f", checkRUBlocksRetVal);
 }
+
+TEST(Local, SimplifyCFGWithNullAC) {
+  LLVMContext Ctx;
+
+  std::unique_ptr<Module> M = parseIR(Ctx,
+                                      R"(
+    declare void @true_path()
+    declare void @false_path()
+    declare void @llvm.assume(i1 %cond);
+
+    define i32 @foo(i1, i32) {
+    entry:
+      %cmp = icmp sgt i32 %1, 0
+      br i1 %cmp, label %if.bb1, label %then.bb1
+    if.bb1:
+      call void @true_path()
+      br label %test.bb
+    then.bb1:
+      call void @false_path()
+      br label %test.bb
+    test.bb:
+      %phi = phi i1 [1, %if.bb1], [%0, %then.bb1]
+      call void @llvm.assume(i1 %0)
+      br i1 %phi, label %if.bb2, label %then.bb2
+    if.bb2:
+      ret i32 %1
+    then.bb2:
+      ret i32 0
+    }
+  )");
+
+  Function &F = *cast<Function>(M->getNamedValue("foo"));
+  TargetTransformInfo TTI(M->getDataLayout());
+
+  SimplifyCFGOptions Options{};
+  Options.setAssumptionCache(nullptr);
+  
+  BasicBlock *TestBB = nullptr;
+  for (BasicBlock &BB : F) {
+    if (isa<PHINode>(&*BB.begin())) {
+      TestBB = &BB;
+      break;
+    }
+  }
+  bool Changed = simplifyCFG(TestBB, TTI, Options);
+  
+  EXPECT_TRUE(Changed);
+}