[LoopInfo] The assertion in getLoopGuardBranch can be a 'return nullptr' under if condition.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Analysis/LoopInfo.cpp | ||
---|---|---|
364 ↗ | (On Diff #214614) | [style] new line after return statement. |
369 ↗ | (On Diff #214614) | 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. |
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.
Would it be possible to add tests for the new cases that are supported now to LoopInfoTest.cpp?
@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?
@DTharun Please add EXPECT_EQ(L->getLoopGuardBranch(), nullptr); to all test cases which has non simplified loops or non rotated loops in LoopInfoTest.cpp.
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.
lib/Analysis/LoopInfo.cpp | ||
---|---|---|
373 ↗ | (On Diff #218636) | You want isLoopExiting(Latch)....so change to if (!isLoopExiting(Latch)) return; Please undo all your changes to the test cases. |
lib/Analysis/LoopInfo.cpp | ||
---|---|---|
373 ↗ | (On Diff #218636) | Ohh, thanks for the catch. I will fix it. |
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 */ }
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.
unittests/Analysis/LoopInfoTest.cpp | ||
---|---|---|
1157 ↗ | (On Diff #220363) | That's right, I meant the data layout. |
LGTM thanks. Sorry for the delay.
lib/Analysis/LoopInfo.cpp | ||
---|---|---|
368 ↗ | (On Diff #220991) | please remove the unrelated newline insertion here. |