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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

A few comments/questions.



================
Comment at: mlir/lib/Target/LLVMIR/CMakeLists.txt:60
   MLIRROCDLToLLVMIRTranslation
+  MLIROpenMPDialect
   )
----------------
Is this required?


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


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


================
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,
----------------
Ideally, this code should be in `OpenMPToLLVMIRTranslation.cpp`. Can this be moved there?


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

https://reviews.llvm.org/D147172