[Openmp-commits] [PATCH] D149368: [MLIR][OpenMP] Initial Lowering of Declare Target for Data

[Sergio Afonso via Phabricator via Openmp-commits]

skatrak added a comment.

Thanks Andrew for the changes. Just a couple of nits, but overall LGTM.



================
Comment at: flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp:44
+  static bool isAddressOfGlobalDeclareTarget(mlir::Value value) {
+    if (value.getDefiningOp())
+      if (fir::AddrOfOp addressOfOp =
----------------
Just like in the other spot I suggested the same change, this line and the next can be simplified like this.


================
Comment at: flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp:180
     // was the original intent of the pass.
-    for (auto oper : targetOp.getOperation()->getOperands()) {
+    for (mlir::Value oper : targetOp.getOperation()->getOperands()) {
       if (auto mapEntry =
----------------



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1571
+static llvm::Value *
+getRefPtrIfDeclareTarget(mlir::Value const value,
+                         LLVM::ModuleTranslation &moduleTranslation) {
----------------
I think we can remove `const` here: https://mlir.llvm.org/docs/Rationale/UsageOfConst/


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1668
+    auto mapFlag = llvm::omp::OpenMPOffloadMappingFlags(
+        mapTypes[index].dyn_cast<IntegerAttr>().getUInt());
 
----------------
If we can assert at this point that `mapTypes` can only contain `IntegerAttr` at this point, it's better to do `mapTypes[index].cast<IntegerAttr>`. Otherwise, we should check the result of the `dyn_cast` before calling `getUInt` on it to avoid a crash.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2175
+          ompBuilder->getTargetEntryUniqueInfo(fileInfoCallBack), mangledName,
+          generatedRefs, /*OpenMPSimd*/false, targetTriple, /*GlobalInitializer*/nullptr, /*VariableLinkage*/nullptr, gVal->getType(),
+          gVal);
----------------
Seems like clang-format missed breaking this line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149368