[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: mlir/lib/Target/LLVMIR/CMakeLists.txt:60
   MLIRROCDLToLLVMIRTranslation
+  MLIROpenMPDialect
   )
----------------
kiranchandramohan wrote:
> Is this required?
> Is this required?

No, thanks for pointing this out. I think it was needed in an earlier iteration.


================
Comment at: mlir/lib/Target/LLVMIR/CMakeLists.txt:88
   MLIRLLVMIRToLLVMTranslation
+  MLIROpenMPDialect
   )
----------------
kiranchandramohan wrote:
> Is this required?
> Is this required?

Not needed here either. Thanks!


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1643-1644
+
+  llvm::SetVector<Value> operandSet;
+  getUsedValuesDefinedAbove(targetRegion, operandSet);
+
----------------
kiranchandramohan wrote:
> Can this function be sunk into the OpenMPIRBuilder? If it is just a call like this then please consider, otherwise ignore.
> Can this function be sunk into the OpenMPIRBuilder? If it is just a call like this then please consider, otherwise ignore.

No, unfortunately it cannot be replaced with a simple function call.



================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:1327-1335
+  bool isDevice = false;
+  if (auto offloadMod = dyn_cast<mlir::omp::OffloadModuleInterface>(module))
+    isDevice = offloadMod.getIsDevice();
+
+  // TODO: set the flags when available
+  llvm::OpenMPIRBuilderConfig Config(isDevice, /* IsTargetCodegen */ false,
+                                     /* HasRequiresUnifiedSharedMemory */ false,
----------------
kiranchandramohan wrote:
> Ideally, this code should be in `OpenMPToLLVMIRTranslation.cpp`. Can this be moved there?
> Ideally, this code should be in `OpenMPToLLVMIRTranslation.cpp`. Can this be moved there?

I'm not sure how to do that. The code in OpenMPToLLVMIRTranslation code implements an interface and does not have an internal state, and we probably don't want to set the config in every function. The object that holds the state is ModuleTranslation. We could move this code to the constructor of the ModuleTranslation class if you prefer.


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

https://reviews.llvm.org/D147172