Page MenuHomePhabricator

Remove asserts in getLoopGuardBranch
ClosedPublic

Authored by DTharun on Aug 12 2019, 5:23 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

DTharun created this revision.Aug 12 2019, 5:23 AM
DTharun updated this revision to Diff 214614.Aug 12 2019, 5:53 AM
DTharun retitled this revision from Remove assert in getLoopGuardBranch to Remove asserts in getLoopGuardBranch.
Whitney added inline comments.Aug 12 2019, 11:31 AM
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.

DTharun updated this revision to Diff 214765.Aug 12 2019, 10:59 PM
Whitney accepted this revision.Aug 13 2019, 6:27 AM
This revision is now accepted and ready to land.Aug 13 2019, 6:27 AM

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.

Whitney requested changes to this revision.Aug 13 2019, 6:29 AM
This revision now requires changes to proceed.Aug 13 2019, 6:29 AM
fhahn added a subscriber: fhahn.Aug 13 2019, 6:30 AM

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

DTharun marked 2 inline comments as done.Aug 13 2019, 6:30 AM
This comment was removed by DTharun.
DTharun updated this revision to Diff 214826.Aug 13 2019, 6:45 AM

Reverted the Preheader and Latch check to assert.

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

Whitney accepted this revision.EditedAug 21 2019, 6:06 AM

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

This revision is now accepted and ready to land.Aug 21 2019, 6:06 AM

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

fhahn added a comment.Aug 23 2019, 8:24 AM

@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.

DTharun updated this revision to Diff 218636.Sep 4 2019, 3:46 AM

Updated LoopInfoTest.cpp

A gentle reminder.

fhahn accepted this revision.Sep 10 2019, 7:51 AM

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 requested changes to this revision.Sep 11 2019, 2:46 PM
Whitney added inline comments.
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.
And add EXPECT_EQ(L->getLoopGuardBranch(), nullptr); to all test cases which has non simplified loops or non rotated loops in LoopInfoTest.cpp.

This revision now requires changes to proceed.Sep 11 2019, 2:46 PM
DTharun marked an inline comment as done.Sep 11 2019, 10:06 PM
DTharun added inline comments.
lib/Analysis/LoopInfo.cpp
373 ↗(On Diff #218636)

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

DTharun updated this revision to Diff 219851.Sep 12 2019, 12:49 AM
DTharun edited the summary of this revision. (Show Details)

No test cases fail from LoopInfoTest.cpp.

fhahn requested changes to this revision.Sep 12 2019, 1:07 AM

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 */ }
This revision now requires changes to proceed.Sep 12 2019, 1:07 AM

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.

DTharun updated this revision to Diff 220363.Sep 16 2019, 11:14 AM
Whitney added inline comments.Sep 18 2019, 9:01 AM
unittests/Analysis/LoopInfoTest.cpp
1157 ↗(On Diff #220363)

is this needed?

1185 ↗(On Diff #220363)

assert always come with a message.

DTharun marked 2 inline comments as done.Sep 18 2019, 10:04 PM
DTharun added inline comments.
unittests/Analysis/LoopInfoTest.cpp
1157 ↗(On Diff #220363)

Do you mean the data layout?

1185 ↗(On Diff #220363)

Ok.

Whitney added inline comments.Sep 19 2019, 6:33 AM
unittests/Analysis/LoopInfoTest.cpp
1157 ↗(On Diff #220363)

That's right, I meant the data layout.

DTharun updated this revision to Diff 220991.Sep 20 2019, 3:14 AM
Whitney accepted this revision.Sep 20 2019, 7:36 AM

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

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

@fhahn
Gentle reminder ...

fhahn accepted this revision.Oct 4 2019, 11:04 AM

LGTM thanks. Sorry for the delay.

lib/Analysis/LoopInfo.cpp
368 ↗(On Diff #220991)

please remove the unrelated newline insertion here.

This revision is now accepted and ready to land.Oct 4 2019, 11:04 AM
Closed by commit rL373857: [LOOPGUARD] Remove asserts in getLoopGuardBranch (authored by whitneyt, committed by ). · Explain WhyOct 6 2019, 9:40 AM
This revision was automatically updated to reflect the committed changes.