[PATCH] D147172: [OpenMP][Flang][MLIR] Add lowering of TargetOp for host codegen

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks for the changes. I have a few comments or questions. See inline.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1836-1838
+  /// \param EntryInfo The entry information about the function
+  /// \param NumTeams Number of teams specified in the num_teams clause
+  /// \param NumThreads Number of teams specified in the thread_limit clause
----------------
Nit: Please add a full stop/dot at the end for all the three above to keep it consistent.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4118
+                       OpenMPIRBuilder::TargetBodyGenCallbackTy &CBFunc) {
+  llvm::SmallVector<llvm::Type *, 4> ParameterTypes;
+  for (auto &Arg : Inputs) {
----------------
Nit: Is 4 significant here?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4119-4121
+  for (auto &Arg : Inputs) {
+    ParameterTypes.push_back(Arg->getType());
+  }
----------------
Nit: braces not required.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4120
+  for (auto &Arg : Inputs) {
+    ParameterTypes.push_back(Arg->getType());
+  }
----------------
In the past we have seen that if the number of live-ins/parameters go above a certain number that leads to ABI issues on some platforms.
The OpenMPIRBuilder flow packs the arguments into a struct and then unpacks it in the outlined function. Is this not a concern for offloading?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4141-4142
+  for (auto InArg : zip(Inputs, Func->args())) {
+    auto Input = std::get<0>(InArg);
+    auto &Arg = std::get<1>(InArg);
+    // Collect all the instructions
----------------
Nit: spell the types.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4144-4151
+    for (User *User : make_early_inc_range(Input->users())) {
+      if (auto Instr = dyn_cast<Instruction>(User)) {
+        if (Instr->getFunction() == Func) {
+          Instr->replaceUsesOfWith(Input, &Arg);
+        }
+      }
+    }
----------------
Nit: braces not required.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4196
+  emitTargetCall(Builder, OutlinedFn, Args);
+  Builder.GetInsertBlock()->getParent()->getParent()->dump();
+
----------------
Nit: Is this leftover debug code?


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1571
+    opInst.emitError("If clause not yet supported");
+    return false;
+  }
----------------
Is this required? If so, please add it to other `if` statements as well.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1602
+  if (isDevice) // TODO: Implement device codegen.
+    return success();
+
----------------
Return success or failure?


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1612-1614
+  for (Value operand : operandSet) {
+    inputs.push_back(moduleTranslation.lookupValue(operand));
+  }
----------------
Nit: braces not required.


================
Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:177
 // -----
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<i64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f80, dense<128> : vector<2xi32>>, #dlti.dl_entry<i1, dense<8> : vector<2xi32>>, #dlti.dl_entry<i8, dense<8> : vector<2xi32>>, #dlti.dl_entry<i16, dense<16> : vector<2xi32>>, #dlti.dl_entry<i32, dense<32> : vector<2xi32>>, #dlti.dl_entry<f16, dense<16> : vector<2xi32>>, #dlti.dl_entry<f64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f128, dense<128> : vector<2xi32>>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_device = #omp.isdevice<is_device = false>} {
----------------
Please also add a trivial test for an OpenMP construct that can occur inside the target region, just to ensure that the target region can contain other constructs and it works OK.

Something very simple like the following.
```
omp.target {
  omp.parallel {
     ....
     omp.terminator
  }
  omp.terminator
}
```


================
Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:178
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<i64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f80, dense<128> : vector<2xi32>>, #dlti.dl_entry<i1, dense<8> : vector<2xi32>>, #dlti.dl_entry<i8, dense<8> : vector<2xi32>>, #dlti.dl_entry<i16, dense<16> : vector<2xi32>>, #dlti.dl_entry<i32, dense<32> : vector<2xi32>>, #dlti.dl_entry<f16, dense<16> : vector<2xi32>>, #dlti.dl_entry<f64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f128, dense<128> : vector<2xi32>>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_device = #omp.isdevice<is_device = false>} {
+  llvm.func @omp_target_region_() {
----------------
Nit: If the `dlti` is not required for this patch, then you can consider omitting it.


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

https://reviews.llvm.org/D147172