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

[Jan Sjödin via Phabricator via llvm-commits]

jsjodin added inline comments.


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

No, I picked something small that hopefully doesn't require a resize. Not sure if there is a better value to pick?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4120
+  for (auto &Arg : Inputs) {
+    ParameterTypes.push_back(Arg->getType());
+  }
----------------
kiranchandramohan wrote:
> 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?
> 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?

Clang does not pack arguments into a struct for the outlined fallback (host) for target regions. It could be that the number of parameters are fewer compared to other outlining cases. 
It might be desirable to use the kernel args structure so both the host and device use the same argument passing, but this is not done today.


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


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1571
+    opInst.emitError("If clause not yet supported");
+    return false;
+  }
----------------
kiranchandramohan wrote:
> Is this required? If so, please add it to other `if` statements as well.
Yes, I think it is okay to error out. I will add it to the other cases.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1602
+  if (isDevice) // TODO: Implement device codegen.
+    return success();
+
----------------
kiranchandramohan wrote:
> Return success or failure?
Success for now. Basically ignore this so we can get through the driver. There is a follow up patch https://reviews.llvm.org/D147940 that will allow us to enable device codegen.


================
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>} {
----------------
kiranchandramohan wrote:
> 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
> }
> ```
> 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
> }
> ```

Sure, I can add a test for this.


================
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_() {
----------------
kiranchandramohan wrote:
> Nit: If the `dlti` is not required for this patch, then you can consider omitting it.
> Nit: If the `dlti` is not required for this patch, then you can consider omitting it.

We can remove dlti it for this patch.


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

https://reviews.llvm.org/D147172