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

[Andrew Gozillon via Phabricator via Openmp-commits]

agozillon added a comment.

Made an attempt at addressing all review points made by @skatrak in the prior patch as well as rebase on most recent upstream changes. Let me know what you think when you have a spare moment @skatrak!



================
Comment at: flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp:33
+  bool isDeclareTargetOp(mlir::Operation *op) {
+    if (fir::AddrOfOp addressOfOp = mlir::dyn_cast<fir::AddrOfOp>(op))
+      if (fir::GlobalOp gOp = mlir::dyn_cast<fir::GlobalOp>(
----------------
skatrak wrote:
> Nit: Perhaps for consistency I would swap out `fir::AddrOfOp` and `fir::GlobalOp` in the declared variable names here for `auto`, and replace `auto` for `mlir::Value` in the two new loops below over `inputs` and `targetOp.getMapOperands()`, so that you only use `auto` where the type is already spelled out in the initialization.
rebased on the recent map changes I upstreamed, so the changeset here isn't required anymore! but I've taken into account these review points and done my best to apply them none the less, as they're good suggestions for tidying it up a bit more.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2055
 
-static LogicalResult
-convertDeclareTargetAttr(Operation *op,
-                         omp::DeclareTargetAttr declareTargetAttr,
+LogicalResult
+convertDeclareTargetAttr(Operation *op, mlir::omp::DeclareTargetAttr attribute,
----------------
skatrak wrote:
> Is there any reason to make this function visible outside the translation unit?
minor rebase issue on my end from when you added the function upstream before me I think! no reason for it not to be static thank you for the catch.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2111
+        return std::pair<std::string, uint64_t>(
+            llvm::StringRef(loc.getFilename()), loc.getLine());
+      };
----------------
skatrak wrote:
> Can you provide a default to fail gracefully if `loc` turns out to be null? Or throw out an error earlier when it's initialized, if that's not possible.
The call to getTargetEntryUniqueInfo that uses it emits an error when it can't generate a unique id from the file data given, so I've attempted to emit some empty information in this case so that it fails gracefully (and silently) here and the error from the OMPIRBuilder function is emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149368