[Openmp-commits] [PATCH] D149368: [MLIR][OpenMP] Initial Lowering of Declare Target for Data
Sergio Afonso via Phabricator via Openmp-commits
openmp-commits at lists.llvm.org
Tue Sep 19 05:24:38 PDT 2023
skatrak added a comment.
Thank you Andrew for this work! I've got a few comments, but most of them are just style-related or pointing out typos.
================
Comment at: flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp:32
+ bool isDeclareTargetOp(mlir::Operation *op) {
+ if (fir::AddrOfOp addressOfOp = mlir::dyn_cast<fir::AddrOfOp>(op))
----------------
Maybe rename this to `isAddressOfGlobalDeclareTarget` or similar, pass in the `mlir::Value` and check for the defining op inside this function. It should reduce some code duplication among the two calls to this function and also make its purpose clearer.
================
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>(
----------------
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.
================
Comment at: flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp:94
+ // target has its addressOfOp cloned over, whereas we skip it for
+ // regular map variables. This is because We need knowledge of
+ // which global is linked to the map operation for declare
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1548
+static llvm::Value *
+getRefPtrIfDeclareTarget(mlir::Value const &value,
+ LLVM::ModuleTranslation &moduleTranslation) {
----------------
Nit: It should be fine to pass by value here.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1553
+ // An easier way to do this may just be to keep track of any pointer
+ // references and there mapping to their respective operation
+ if (isa_and_nonnull<LLVM::AddressOfOp>(value.getDefiningOp())) {
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1554
+ // references and there mapping to their respective operation
+ if (isa_and_nonnull<LLVM::AddressOfOp>(value.getDefiningOp())) {
+ LLVM::AddressOfOp addressOfOp =
----------------
The two calls to `getDefiningOp` I think can be simplified like this.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1557
+ dyn_cast<LLVM::AddressOfOp>(value.getDefiningOp());
+ LLVM::GlobalOp gOp = dyn_cast<LLVM::GlobalOp>(
+ addressOfOp->getParentOfType<mlir::ModuleOp>().lookupSymbol(
----------------
This should be inside of an 'if' statement, since the returned value is accessed right after with no presence checks.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1567
+ // declare target operation, similar to Clang
+ if (declareTargetGlobal.isDeclareTarget() &&
+ ((declareTargetGlobal.getDeclareTargetCaptureClause() ==
----------------
I think this check is redundant. By comparing the result of `declareTargetGlobal.getDeclareTargetCaptureClause()` against some non-null values we're already making sure that the condition won't be satisfied if there is no `omp.declare_target` attribute.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1573
+ ompBuilder->Config.hasRequiresUnifiedSharedMemory()))) {
+ llvm::SmallString<64> suffix;
+ {
----------------
Can you refactor this suffix generation code into a function? I think it would help with readability here.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1657
+ // with OMP_MAP_PTR_AND_OBJ instead.
+ if (isTargetParams && !refPtr)
+ mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM;
----------------
Nit: It might be more readable to structure it as:
```
if (refPtr)
mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PTR_AND_OBJ;
else if (isTargetParams)
mapFlag |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TARGET_PARAM;
```
But feel free to ignore if you disagree.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1940
+static void
+handleDeclareTargetMapVar(llvm::SmallVector<Value> &mapOperands,
+ LLVM::ModuleTranslation &moduleTranslation,
----------------
I think `llvm::ArrayRef<mlir::Value>` is better suited here.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1943
+ llvm::IRBuilderBase &builder) {
+ for (const auto &mapOp : mapOperands) {
+ llvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1947
+ getRefPtrIfDeclareTarget(mapOp, moduleTranslation)) {
+ // The users iterator will get invalidated if we modify an element,
+ // so we populate this vector of uses to alter each user on an individual
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1949
+ // so we populate this vector of uses to alter each user on an individual
+ // basis to emit it's own load (rather than one load for all).
+ llvm::SmallVector<llvm::User *> userVec;
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1954
+
+ for (auto *user : userVec) {
+ if (auto *insn = dyn_cast<llvm::Instruction>(user)) {
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1982
+ auto getKernelArguments = [&](const llvm::SetVector<Value> &operandSet,
+ llvm::SmallVector<llvm::Value *> &llvmInputs) {
+ for (Value operand : operandSet) {
----------------
According to the programmer's manual [here](https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h), it is preferred to avoid hardcoding the "small" size of small vectors when used as output parameters.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1984
+ for (Value operand : operandSet) {
+ if (getRefPtrIfDeclareTarget(operand, moduleTranslation))
+ continue;
----------------
I think it's more readable to negate the condition rather than adding a `continue` statement here, but feel free to ignore this comment if you disagree.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2047
+ // Remap access operations to declare target reference pointers for the
+ // device, essentially generating extra loadop's as neccesary
+ if (moduleTranslation.getOpenMPBuilder()->Config.isTargetDevice()) {
----------------
================
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,
----------------
Is there any reason to make this function visible outside the translation unit?
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2103
+ auto targetTripleAttr =
+ op->getParentOfType<mlir::ModuleOp>().getOperation()->getAttr(
+ LLVM::LLVMDialect::getTargetTripleAttrName());
----------------
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2107
+ targetTriple.emplace_back(
+ targetTripleAttr.dyn_cast_or_null<mlir::StringAttr>().data());
+
----------------
This seems like it would crash if `targetTripleAttr` was not present or a `StringAttr`, since the output of `dyn_cast_or_null` is dereferenced without checking first.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2111
+ return std::pair<std::string, uint64_t>(
+ llvm::StringRef(loc.getFilename()), loc.getLine());
+ };
----------------
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.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:2117
+ ompBuilder->getTargetEntryUniqueInfo(fileInfoCallBack), mangledName,
+ generatedRefs, false, targetTriple, nullptr, nullptr, gVal->getType(),
+ gVal);
----------------
Please add comments with the parameter names for the ones set to `false` or `nullptr` here and in the call below to `getAddrOfDeclareTargetVar()`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149368/new/
https://reviews.llvm.org/D149368
More information about the Openmp-commits
mailing list