[PATCH] D28724: Use getLoopLatch in place of isLoopSimplifyForm

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Some comments on the test case.



================
Comment at: unittests/Analysis/LoopInfoTest.cpp:20
+
+static SmallVector<MDNode *, 1> LoopIDVector;
+
----------------
I'd rather put this inside the `LoopInfoTest` struct as a public instance variable.


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:30
+    FI++; // First basic block is entry - skip it.
+    BasicBlock *Header = &*FI++;
+    Loop *L = LI->getLoopFor(Header);
----------------
Can you `ASSERT` or `assert` (up to you which one) that the name of `Header` is `for.cond`, just to make sure we're doing what we think we're doing?


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:52
+
+std::unique_ptr<Module> makeLLVMModule(LLVMContext &Context,
+                                       const char *ModuleStr) {
----------------
Make this a `static` function.


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:66
+         "entry:\n"
+         "  br i1 undef, label %for.cond, label %for.end\n"
+         "for.cond:\n"
----------------
This is totally up to you, but I tend to prefer not using explicit `"\n"` s but instead end lines with a space.  That makes the string more readable and it works fine since LLVM is (mostly?) whitespace insensitive.


================
Comment at: unittests/Analysis/LoopInfoTest.cpp:81
+  LoopInfoTest *P = new LoopInfoTest();
+  LLVMContext Context;
+  std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleStr);
----------------
Creating a pass for this seems unnecessary -- why can't you directly call `LoopInfo::analyze` on the relevant function?


https://reviews.llvm.org/D28724