Remove asserts in getLoopGuardBranch

[DTharun]

Summary

[LoopInfo] The assertion in getLoopGuardBranch can be a 'return nullptr' under if condition.

[Whitney]

lib/Analysis/LoopInfo.cpp

364

[style] new line after return statement.

369

https://reviews.llvm.org/D65958 Add isRotated method to Loop class. Would be nice to use that instead. (but that is not landed yet)

[style] Upper case to start a new sentence, or change the period to a comma after latch.

[Whitney]

actually the check for preheader and latch should change back to an assert, as we already checked that the loop is in simplify form, so there must exists a preheader and latch.

[fhahn]

Would it be possible to add tests for the new cases that are supported now to LoopInfoTest.cpp?

[DTharun]

This comment was removed by DTharun.

[DTharun]

Reverted the Preheader and Latch check to assert.

[Whitney]

@fhahn Most of the test cases in LoopInfoTest.cpp has checks for getLoopGuardBranch(). Are you talking about LoopUniqueExitBlocks and LoopNonLatchUniqueExitBlocks?

[Whitney]

LGTM. If there are no further comments from @fhahn, I will help @DTharun to commit it on Friday.

[DTharun]

@Whitney I don't have commit rights, please commit it.
Thanks for reviewing.

[fhahn]

In D66084#1627061, @Whitney wrote:

@fhahn Most of the test cases in LoopInfoTest.cpp has checks for getLoopGuardBranch(). Are you talking about LoopUniqueExitBlocks and LoopNonLatchUniqueExitBlocks?

Sure, but eh behaviour of getLopGuardBranch changed, so the new behaviour should be checked in the tests?

[Whitney]

@DTharun Please add EXPECT_EQ(L->getLoopGuardBranch(), nullptr); to all test cases which has non simplified loops or non rotated loops in LoopInfoTest.cpp.

[DTharun]

Updated LoopInfoTest.cpp

[DTharun]

A gentle reminder.

[fhahn]

LGTM. It would be good if you could prefix the title of the commit with something like [LoopInfo]. In the future, please upload the diff with full context.

[Whitney]

lib/Analysis/LoopInfo.cpp

373

You want isLoopExiting(Latch)....so change to if (!isLoopExiting(Latch)) return;

Please undo all your changes to the test cases.
And add EXPECT_EQ(L->getLoopGuardBranch(), nullptr); to all test cases which has non simplified loops or non rotated loops in LoopInfoTest.cpp.

[DTharun]

lib/Analysis/LoopInfo.cpp

373

Ohh, thanks for the catch. I will fix it.

[DTharun]

No test cases fail from LoopInfoTest.cpp.

[fhahn]

Sorry for not being more clear. I meant we need a test where the latch is not exiting, which without this patch will assert and with this patch will return nullptr. So please add a test for something like

+TEST(LoopInfoTest, LatchNotExiting) {
+  const char *ModuleStr =
+      "define void @foo(i32* %A, i32 %ub) {\n"
+      "entry:\n"
+      "  %guardcmp = icmp slt i32 0, %ub\n"
+      "  br i1 %guardcmp, label %for.preheader, label %for.end\n"
+      "for.preheader:\n"
+      "  br label %for.body\n"
+      "for.body:\n"
+      "  %i = phi i32 [ 0, %for.preheader ], [ %inc, %for.body ]\n"
+      "  %aux = phi i32 [ 0, %for.preheader ], [ %auxinc, %for.body ]\n"
+      "  %loopvariant = phi i32 [ 0, %for.preheader ], [ %loopvariantinc, %for.body ]\n"
+      "  %usedoutside = phi i32 [ 0, %for.preheader ], [ %usedoutsideinc, %for.body ]\n"
+      "  %mulopcode = phi i32 [ 0, %for.preheader ], [ %mulopcodeinc, %for.body ]\n"
+      "  %idxprom = sext i32 %i to i64\n"
+      "  %arrayidx = getelementptr inbounds i32, i32* %A, i64 %idxprom\n"
+      "  store i32 %i, i32* %arrayidx, align 4\n"
+      "  %mulopcodeinc = mul nsw i32 %mulopcode, 5\n"
+      "  %usedoutsideinc = add nsw i32 %usedoutside, 5\n"
+      "  %loopvariantinc = add nsw i32 %loopvariant, %i\n"
+      "  %auxinc = add nsw i32 %aux, 5\n"
+      "  %inc = add nsw i32 %i, 1\n"
+      "  %cmp = icmp slt i32 %inc, %ub\n"
+      "  br i1 %cmp, label %for.latch, label %for.exit\n"
+      "for.latch:\n"
+      "  br label %for.body\n"
+      "for.exit:\n"
+      "  %lcssa = phi i32 [ %usedoutside, %for.body ]\n"
+      "  br label %for.end\n"
+      "for.end:\n"
+      "  ret void\n"
+      "}\n";
+
+  // Parse the module.
+  LLVMContext Context;
+  std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleStr);
+
+  runWithLoopInfoPlus(
+      *M, "foo",
+      [&](Function &F, LoopInfo &LI, ScalarEvolution &SE) { /* Add checks */ }

[Whitney]

or you can add EXPECT_EQ(L->getLoopGuardBranch(), nullptr); to existing tests, e.g. LoopWithSingleLatch....in that test case, you can see that the loop is not rotated, i.e. the latch for.inc is not the exiting block, the exiting block is for.cond.

[Whitney]

unittests/Analysis/LoopInfoTest.cpp

1157 ↗

(On Diff #220363)

is this needed?

1185 ↗

(On Diff #220363)

assert always come with a message.

[DTharun]

unittests/Analysis/LoopInfoTest.cpp

1157 ↗

(On Diff #220363)

Do you mean the data layout?

1185 ↗

(On Diff #220363)

Ok.

[Whitney]

unittests/Analysis/LoopInfoTest.cpp

1157 ↗

(On Diff #220363)

That's right, I meant the data layout.

[DTharun]

Thanks for reviewing @Whitney @fhahn.
Please commit it, I don't have commit rights.

[Whitney]

@DTharun I can help you to commit this patch after @fhahn approves it.

[DTharun]

@fhahn
Gentle reminder ...

[fhahn]

LGTM thanks. Sorry for the delay.

lib/Analysis/LoopInfo.cpp

368

please remove the unrelated newline insertion here.

lib/Analysis/LoopInfo.cpp

 }
 
 BranchInst *Loop::getLoopGuardBranch() const {
-  assert(isLoopSimplifyForm() && "Only valid for loop in simplify form");
+  if (!isLoopSimplifyForm())
+    return nullptr;
+
   BasicBlock *Preheader = getLoopPreheader();
   BasicBlock *Latch = getLoopLatch();
-  assert(Preheader && Latch &&
-         "Expecting a loop with valid preheader and latch");
-  assert(isLoopExiting(Latch) && "Only valid for rotated loop");
+
+  // Expecting a loop with valid preheader and latch,
+  // and loop should be in rotate form.
+  if (!Preheader || !Latch || !isLoopExiting(Latch))
+    return nullptr;
 
   Instruction *LatchTI = Latch->getTerminator();
   if (!LatchTI || LatchTI->getNumSuccessors() != 2)