[PATCH] D90830: [OpenMPIRBuilder] Implement CreateCanonicalLoop.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some minor comments, overall this looks reasonable. I'll let someone else take a look.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:702
+  /// any bit-width.
+  Value *getTripCount() const { return Cond->front().getOperand(1); }
+
----------------
front = getTerminator, right?
Can we add a few more assertions here and elsewhere, just to make sure the structure is as we expect it.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:887
+  CL->assertOK();
+#endif
+  return CL;
----------------
I usually prefer the `assert(CL->isValid() && "...")` style but no objection at the end of the day. (the reason is the other one can be used to dump more of the context easily, e.g., `if (!isValid()) dump_stuff();`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:903
+  assert(IndVarTy == Stop->getType());
+  assert(IndVarTy == Step->getType());
+
----------------
Nit : `&& "Loop counter type mismatch"` or similar.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:958
+void CanonicalLoopInfo::eraseFromParent() {
+  assert(IsValid);
+  IsValid = false;
----------------
Nit: msg.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:978
+  for (auto BB : BBsToRemove)
+    BB->eraseFromParent();
+}
----------------
I thought we have an API to delete blocks without the need for this. Maybe try to use that instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90830/new/

https://reviews.llvm.org/D90830