[PATCH] D155633: [OpenMP][OpenMPIRBuilder] Add kernel launch codegen to emitTargetCall

[Akash Banerjee via Phabricator via llvm-commits]

TIFitis added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4448
+
+  auto MapInfo = GenMapInfoCB(Builder.saveIP());
+  OMPBuilder.emitOffloadingArrays(AllocaIP, Builder.saveIP(), MapInfo, Info,
----------------
Expand type here.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4453
+  OpenMPIRBuilder::TargetDataRTArgs RTArgs;
+  OMPBuilder.emitOffloadingArraysArgument(Builder, RTArgs, Info);
+
----------------
This allows debug info to be accurately generated.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4489
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createTarget(
-    const LocationDescription &Loc, OpenMPIRBuilder::InsertPointTy CodeGenIP,
-    TargetRegionEntryInfo &EntryInfo, int32_t NumTeams, int32_t NumThreads,
-    SmallVectorImpl<Value *> &Args, TargetBodyGenCallbackTy CBFunc) {
+    const LocationDescription &Loc, OpenMPIRBuilder::InsertPointTy AllocaIP,
+    OpenMPIRBuilder::InsertPointTy CodeGenIP, TargetRegionEntryInfo &EntryInfo,
----------------
No need to explicitly specify OpenMPIRBuilder namespace here.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4490
+    const LocationDescription &Loc, OpenMPIRBuilder::InsertPointTy AllocaIP,
+    OpenMPIRBuilder::InsertPointTy CodeGenIP, TargetRegionEntryInfo &EntryInfo,
+    int32_t NumTeams, int32_t NumThreads, SmallVectorImpl<Value *> &Args,
----------------
Same, remove explicit namespace.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:5077-5078
+    uint32_t SrcLocStrSize;
+    combinedInfo.Names.emplace_back(ompBuilder.getOrCreateSrcLocStr(
+        "Unknown loc - stub implementation", SrcLocStrSize));
+    combinedInfo.Types.emplace_back(llvm::omp::OpenMPOffloadMappingFlags(
----------------
Maybe we can replace this with OMPBuilder.getOrCreateDefaultSrcLocStr?

Also it is fine to leave the Names empty if not available.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:5084
+    combinedInfo.Sizes.emplace_back(ompBuilder.Builder.getInt64(
+        ompBuilder.M.getDataLayout().getTypeAllocSize(arg->getType())));
+  }
----------------
Can you please check if this works with function arguments?

AFAIK getTypeAllocSize doesn't work on function arguments, in that case we might want to move this function as a callback to MLIR.


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

https://reviews.llvm.org/D155633