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

[Akash Banerjee via Phabricator via cfe-commits]

TIFitis added inline comments.


================
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:
> 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?
Fixed.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4122
+      Builder.CreateCall(getOrCreateRuntimeFunctionPtr(*MapperFunc),
+                         OffloadingArgs);
+    } else {
----------------
jdoerfert wrote:
> 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.
Added


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4761
+    if (CondConstant)
+      ThenGen(Builder.saveIP(), Builder.saveIP());
+    else
----------------
jdoerfert wrote:
> 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?
The CallBacks were ignoring the AllocaIP passed and getting it from the context instead.
I've updated them to use the CallBack argument instead.

This patch https://reviews.llvm.org/D150860 changes Clang to use this function for TargetDataCall emission which tests most aspects of it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557