[PATCH] D146557: [MLIR][OpenMP] Refactoring createTargetData in OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I think this is now style wise pretty good, I still found some potential problems below.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1487
+  // possible, or else at the end of the function.
+  void emitBlock(BasicBlock *BB, Function *CurFn, bool IsFinished = false);
+
----------------
jdoerfert wrote:
> This does not mention the deletion stuff, etc.
This talks about creating a new block, but reading the function it seems it will just place `\p BB`. Which one is it?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4122
+      Builder.CreateCall(getOrCreateRuntimeFunctionPtr(*MapperFunc),
+                         OffloadingArgs);
+    } else {
----------------
Can we at least assert MapperFunc is non null if standalone is true. It seems like an invariant almost worth documenting, at least with an assertion + message.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4761
+    if (CondConstant)
+      ThenGen(Builder.saveIP(), Builder.saveIP());
+    else
----------------
In the call above you pass the AllocaIP here, now it is both times the saveIP. I doubt this is correct. Allocas will end up in the wrong spot, won't they? Do we have a test for that?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4741
+  if (IsFinished && BB->use_empty()) {
+    delete BB;
+    return;
----------------
TIFitis wrote:
> jdoerfert wrote:
> > you should not just delete BB. eraseFromParent is a better way. That said, it is also weird you would delete something in an "emit" function.
> I've changed it.
> 
> The `emitBlock`, `emitBranch` and `emitIfClause` functions are ported from Clang btw.
> //clang/lib/CodeGen/CGStmt.cpp//
That might be so, but given the name and initial documentation, nobody would have expected this function to delete the block, or at least I would not have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557