[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

Thanks for making the change.
I am still going through the patch, I have a few comments.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1574-1575
+      const LocationDescription &Loc, OpenMPIRBuilder::InsertPointTy CodeGenIP,
+      bool HasRegion, SmallVector<uint64_t> &MapTypeFlags,
+      SmallVector<Constant *> &MapNames, struct MapperAllocas &MapperAllocas,
+      Function *MapperFunc, int64_t DeviceID, Value *IfCond,
----------------
SmallVector -> SmallVectorImpl 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+    SplitBlockAndInsertIfThenElse(IfCond, UI, &ThenTI, &ElseTI);
+    ThenTI->getParent()->setName("omp_if.then");
+    ElseTI->getParent()->setName("omp_if.else");
----------------
There is some recent recommendation to not insert artificial instructions and remove them. The preferred approach is to use `splitBB` function(s) inside the OpenMPIRBuilder. This function works on blocks without terminators. You can consult the `IfCondition` code in `createParallel`.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4983
+  CallInst *TargetDataCall = dyn_cast<CallInst>(&BB->back());
+  BB->dump();
+  EXPECT_NE(TargetDataCall, nullptr);
----------------
Is this a debugging leftover?


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4990
+  EXPECT_TRUE(TargetDataCall->getOperand(2)->getType()->isIntegerTy(32));
+  EXPECT_TRUE(TargetDataCall->getOperand(8)->getType()->isPointerTy());
+}
----------------
Call verifyModule to ensure the IR is well formed.


================
Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h:13-14
+
+#ifndef MLIR_TARGET_LLVMIR_DIALECT_UTILS_H
+#define MLIR_TARGET_LLVMIR_DIALECT_UTILS_H
+
----------------
Nit: The header guards would need changing.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1471
+            mapTypes = enterDataOp.getMapTypes();
+            mapperFunc = ompBuilder->getOrCreateRuntimeFunctionPtr(
+                llvm::omp::OMPRTL___tgt_target_data_begin_mapper);
----------------
Ideally we would not want to create an OpenMP runtime calls in the Translation. This is the job of the OpenMPIRbuilder. I would recommend passing a boolean to the OpenMPIRBuilder and let the IRBuilder pick up the runtime function.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1492
+            mapTypes = exitDataOp.getMapTypes();
+            mapperFunc = ompBuilder->getOrCreateRuntimeFunctionPtr(
+                llvm::omp::OMPRTL___tgt_target_data_end_mapper);
----------------
Same comment as above.


================
Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:1
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
----------------
In MLIR it is preferred to test the minimal set of functionalities. So you will have to minimize the CHECK lines and write them manually. Focus on Checking the runtime call and any important IR inserted by the IRBuilder. Keep the contents of the region to a minimum.
https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914