[llvm-branch-commits] [mlir] [MLIR][OpenMP] Add omp.target_triples attribute to the OffloadModuleInterface (PR #100154)
https://github.com/bhandarkar-pranav approved this pull request. https://github.com/llvm/llvm-project/pull/100154 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Add frontend support for -fopenmp-targets (PR #100155)
https://github.com/bhandarkar-pranav approved this pull request. https://github.com/llvm/llvm-project/pull/100155 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [llvm] [mlir] [MLIR][OpenMP][OMPIRBuilder] Add lowering support for omp.target_triples (PR #100156)
https://github.com/bhandarkar-pranav approved this pull request. Thank you, @skatrak - Just a minor nit and and I opened a linked issue so that the exact linker error message is captured for posterity. https://github.com/llvm/llvm-project/pull/100156 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [llvm] [mlir] [MLIR][OpenMP][OMPIRBuilder] Add lowering support for omp.target_triples (PR #100156)
@@ -7053,13 +7053,28 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitTargetTask( << "\n"); return Builder.saveIP(); } + static void emitTargetCall( OpenMPIRBuilder &OMPBuilder, IRBuilderBase &Builder, OpenMPIRBuilder::InsertPointTy AllocaIP, Function *OutlinedFn, Constant *OutlinedFnID, int32_t NumTeams, int32_t NumThreads, SmallVectorImpl &Args, OpenMPIRBuilder::GenMapInfoCallbackTy GenMapInfoCB, SmallVector Dependencies = {}) { + // emitKernelLaunch bhandarkar-pranav wrote: Minor nit : Consider making it clear in the comment here that this is a host-to-host launch? https://github.com/llvm/llvm-project/pull/100156 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [llvm] [mlir] [MLIR][OpenMP][OMPIRBuilder] Add lowering support for omp.target_triples (PR #100156)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/100156 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [llvm] [mlir] [MLIR][OpenMP][OMPIRBuilder] Add lowering support for omp.target_triples (PR #100156)
@@ -7053,13 +7053,30 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitTargetTask( << "\n"); return Builder.saveIP(); } + static void emitTargetCall( OpenMPIRBuilder &OMPBuilder, IRBuilderBase &Builder, OpenMPIRBuilder::InsertPointTy AllocaIP, Function *OutlinedFn, Constant *OutlinedFnID, int32_t NumTeams, int32_t NumThreads, SmallVectorImpl &Args, OpenMPIRBuilder::GenMapInfoCallbackTy GenMapInfoCB, SmallVector Dependencies = {}) { + // Generate a function call to the host fallback implementation of the target + // region. This is called by the host when no offload entry was generated for + // the target region and when the offloading call fails at runtime. + auto &&EmitTargetCallFallbackCB = bhandarkar-pranav wrote: Just a note here, that `if (!OutlinedFnID)`, then we cannot simply inline a call to `OutlinedFn`. We need to the following check ``` if (!OutlinedFnID) { if(RequiresOuterTargetTask) Builder.restoreIP(emitTargetTask(...)); else Builder.restoreIP(EmitTargetCallFallBackCB(Builder.saveIP())); } ``` But dont change this PR because I'll fold that into my upcoming changes and more importantly, `emitTargetTask` ->`emitKernelLaunch` assert on `OutlinedFnID` [here](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L1088) https://github.com/llvm/llvm-project/pull/100156 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
@@ -285,7 +285,75 @@ argument's type: specific `mlir::Attribute` subclass) will be used instead. - Other attribute types will be represented with their `storageType`. - It will create `Operands` structure for each operation, which is an -empty structure subclassing all operand structures defined for the corresponding `OpenMP_Op`'s clauses. +empty structure subclassing all operand structures defined for the corresponding +`OpenMP_Op`'s clauses. + +### Entry Block Argument-Defining Clauses + +Certain OpenMP clauses introduce in their MLIR representation mappings between +outside values and entry block arguments for the region of the MLIR operation +they are applied to. This enables, for example, the introduction of private bhandarkar-pranav wrote: Please consider ``` This enables, for example, the introduction of private copies of the same underlying variable defined outside the MLIR operation the clause is attached to. ``` https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
@@ -285,7 +285,75 @@ argument's type: specific `mlir::Attribute` subclass) will be used instead. - Other attribute types will be represented with their `storageType`. - It will create `Operands` structure for each operation, which is an -empty structure subclassing all operand structures defined for the corresponding `OpenMP_Op`'s clauses. +empty structure subclassing all operand structures defined for the corresponding +`OpenMP_Op`'s clauses. + +### Entry Block Argument-Defining Clauses + +Certain OpenMP clauses introduce in their MLIR representation mappings between +outside values and entry block arguments for the region of the MLIR operation +they are applied to. This enables, for example, the introduction of private +copies of the same underlying variable. Currently, clauses with this property +can be classified in three main categories: + - Map-like clauses: `map`, `use_device_addr` and `use_device_ptr`. + - Reduction-like clauses: `in_reduction`, `reduction` and `task_reduction`. + - Privatization clause: `private`. + +All three kinds of entry block argument-defining clauses use a similar custom +assembly format representation, only differing based on the different pieces of +information attached to each kind. Below, one example of each is shown: + +```mlir +omp.target map_entries(%x -> %x.m, %y -> %y.m : !llvm.ptr, !llvm.ptr) { + // Use %x.m, %y.m in place of %x and %y... +} + +omp.wsloop reduction(@add.i32 %x -> %x.r, byref @add.f32 %y -> %y.r : !llvm.ptr, !llvm.ptr) { + // Use %x.r, %y.r in place of %x and %y... +} + +omp.parallel private(@x.privatizer %x -> %x.p, @y.privatizer %y -> %y.p : !llvm.ptr, !llvm.ptr) { + // Use %x.p, %y.p in place of %x and %y... +} +``` + +As a consequence of parsing and printing the operation's first region entry +block argument names together with the custom assembly format of these clauses, +entry block arguments (i.e. the `^bb0(...):` line) must not be explicitly +defined for these operations. Additionally, it is not possible to implement this +feature while allowing each clause to be independently parsed and printed, +because they need to be printed/parsed together with the corresponding +operation's first region. They must have a well-defined ordering in which +multiple of these clauses are specified for a given operation, as well. + +The parsing/printing of these clauses together with the region provides the +ability to define entry block arguments directly after the `->`. Forcing a +specific ordering between these clauses makes the block argument ordering +well-defined, which is the property used to easily match each clause with the +entry block arguments defined by it. + +Custom printers and parsers for operation regions based on the entry block +argument-defining clauses they take are implemented based on the +`{parse,print}BlockArgRegion` functions, which take care of the sorting and +formatting of each kind of clause, minimizing code duplication resulting from +this approach. One example of the custom assembly format of an operation taking +the `private` and `reduction` clauses is the following: + +```tablegen +let assemblyFormat = clausesAssemblyFormat # [{ + custom($region, $private_vars, type($private_vars), + $private_syms, $reduction_vars, type($reduction_vars), $reduction_byref, + $reduction_syms) attr-dict +}]; +``` + +The `BlockArgOpenMPOpInterface` has been introduced to simplify the addition and +handling of these kinds of clauses. It holds `numBlockArgs()` +functions that by default return 0, to be overriden by each clause through the +`extraClassDeclaration` property. Based on these functions and the expected +alphabetical sorting between entry block argument-defining clauses, it bhandarkar-pranav wrote: I am assuming the tablegen backend for openmp that you have implemented doesn't do the sorting and the onus for the alphabetical sorting is on the user, correct? If that's the case i think that expectation must be made explicit, either here or (preferably) in [Adding an Operation](https://mlir.llvm.org/docs/Dialects/OpenMPDialect/#adding-an-operation) https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
https://github.com/bhandarkar-pranav commented: LGTM. Given how reviewing docs essentially turns into a series of subjective opinions or preferences, please consider most all of my comments as nits, except the one about alphabetical sorting of clauses. https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
@@ -285,7 +285,75 @@ argument's type: specific `mlir::Attribute` subclass) will be used instead. - Other attribute types will be represented with their `storageType`. - It will create `Operands` structure for each operation, which is an -empty structure subclassing all operand structures defined for the corresponding `OpenMP_Op`'s clauses. +empty structure subclassing all operand structures defined for the corresponding +`OpenMP_Op`'s clauses. + +### Entry Block Argument-Defining Clauses + +Certain OpenMP clauses introduce in their MLIR representation mappings between bhandarkar-pranav wrote: Could you consider the following reordering and slight rewording of the first sentence? ``` In their MLIR representation, certain OpenMP clauses introduce a mapping between values defined outside the operation they are applied to and entry block arguments for the region of that MLIR operation. ``` https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
@@ -285,7 +285,75 @@ argument's type: specific `mlir::Attribute` subclass) will be used instead. - Other attribute types will be represented with their `storageType`. - It will create `Operands` structure for each operation, which is an -empty structure subclassing all operand structures defined for the corresponding `OpenMP_Op`'s clauses. +empty structure subclassing all operand structures defined for the corresponding +`OpenMP_Op`'s clauses. + +### Entry Block Argument-Defining Clauses + +Certain OpenMP clauses introduce in their MLIR representation mappings between +outside values and entry block arguments for the region of the MLIR operation +they are applied to. This enables, for example, the introduction of private +copies of the same underlying variable. Currently, clauses with this property +can be classified in three main categories: bhandarkar-pranav wrote: `s/in three/into three` https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
@@ -285,7 +285,75 @@ argument's type: specific `mlir::Attribute` subclass) will be used instead. - Other attribute types will be represented with their `storageType`. - It will create `Operands` structure for each operation, which is an -empty structure subclassing all operand structures defined for the corresponding `OpenMP_Op`'s clauses. +empty structure subclassing all operand structures defined for the corresponding +`OpenMP_Op`'s clauses. + +### Entry Block Argument-Defining Clauses + +Certain OpenMP clauses introduce in their MLIR representation mappings between +outside values and entry block arguments for the region of the MLIR operation +they are applied to. This enables, for example, the introduction of private +copies of the same underlying variable. Currently, clauses with this property +can be classified in three main categories: + - Map-like clauses: `map`, `use_device_addr` and `use_device_ptr`. + - Reduction-like clauses: `in_reduction`, `reduction` and `task_reduction`. + - Privatization clause: `private`. bhandarkar-pranav wrote: Ultra Nit: I think it should be `Privatization clauses:` even if the set has a solitary element. https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
bhandarkar-pranav wrote: > Thanks for the feedback Pranav, let me know if the changes address your > concerns. Thank you for the changes. This LGTM. https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Document entry block argument-defining clauses (NFC) (PR #109811)
https://github.com/bhandarkar-pranav approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/109811 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [mlir] [mlir][OpenMP] Pack task private variables into a heap-allocated context struct (PR #125307)
https://github.com/bhandarkar-pranav commented: Thank you for the PR, Tom. I have some minor comments that may amount to nitpicking. https://github.com/llvm/llvm-project/pull/125307 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [mlir] [mlir][OpenMP] Pack task private variables into a heap-allocated context struct (PR #125307)
@@ -1730,6 +1730,119 @@ buildDependData(std::optional dependKinds, OperandRange dependVars, } } +namespace { +/// TaskContextStructManager takes care of creating and freeing a structure +/// containing information needed by the task body to execute. +class TaskContextStructManager { +public: + TaskContextStructManager(llvm::IRBuilderBase &builder, + LLVM::ModuleTranslation &moduleTranslation, + MutableArrayRef privateDecls) + : builder{builder}, moduleTranslation{moduleTranslation}, +privateDecls{privateDecls} {} + + /// Creates a heap allocated struct containing space for each private + /// variable. Invariant: privateVarTypes, privateDecls, and the elements of + /// the structure should all have the same order (although privateDecls which + /// do not read from the mold argument are skipped). + void generateTaskContextStruct(); + + /// Create GEPs to access each member of the structure representing a private + /// variable, adding them to llvmPrivateVars. Null values are added where + /// private decls were skipped so that the ordering continues to match the + /// private decls. + void createGEPsToPrivateVars(); + + /// De-allocate the task context structure. + void freeStructPtr(); + + MutableArrayRef getLLVMPrivateVars() { +return llvmPrivateVars; + } + + llvm::Value *getStructPtr() { return structPtr; } + +private: + llvm::IRBuilderBase &builder; + LLVM::ModuleTranslation &moduleTranslation; + MutableArrayRef privateDecls; + + /// The type of each member of the structure, in order. + SmallVector privateVarTypes; + + /// LLVM values for each private variable, or null if that private variable is + /// not included in the task context structure + SmallVector llvmPrivateVars; + + /// A pointer to the structure containing context for this task. + llvm::Value *structPtr = nullptr; + /// The type of the structure + llvm::Type *structTy = nullptr; +}; +} // namespace + +void TaskContextStructManager::generateTaskContextStruct() { + if (privateDecls.empty()) +return; + privateVarTypes.reserve(privateDecls.size()); + + for (omp::PrivateClauseOp &privOp : privateDecls) { +// Skip private variables which can safely be allocated and initialised +// inside of the task +if (!privOp.readsFromMold()) + continue; +Type mlirType = privOp.getType(); +privateVarTypes.push_back(moduleTranslation.convertType(mlirType)); + } + + structTy = llvm::StructType::get(moduleTranslation.getLLVMContext(), + privateVarTypes); + + llvm::DataLayout dataLayout = + builder.GetInsertBlock()->getModule()->getDataLayout(); + llvm::Type *intPtrTy = builder.getIntPtrTy(dataLayout); + llvm::Constant *allocSize = llvm::ConstantExpr::getSizeOf(structTy); + + // Heap allocate the structure + structPtr = builder.CreateMalloc(intPtrTy, structTy, allocSize, + /*ArraySize=*/nullptr, /*MallocF=*/nullptr, + "omp.task.context_ptr"); +} + +void TaskContextStructManager::createGEPsToPrivateVars() { + if (!structPtr) { +assert(privateVarTypes.empty()); +return; + } + + // Create GEPs for each struct member and initialize llvmPrivateVars to point + llvmPrivateVars.clear(); + llvmPrivateVars.reserve(privateVarTypes.size()); bhandarkar-pranav wrote: Since we store `nullptr` for private decls that dont read from the mold argument, we might as well reserve `privateDecls.size()` https://github.com/llvm/llvm-project/pull/125307 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [mlir] [mlir][OpenMP] Pack task private variables into a heap-allocated context struct (PR #125307)
@@ -1730,6 +1730,119 @@ buildDependData(std::optional dependKinds, OperandRange dependVars, } } +namespace { +/// TaskContextStructManager takes care of creating and freeing a structure +/// containing information needed by the task body to execute. +class TaskContextStructManager { +public: + TaskContextStructManager(llvm::IRBuilderBase &builder, + LLVM::ModuleTranslation &moduleTranslation, + MutableArrayRef privateDecls) + : builder{builder}, moduleTranslation{moduleTranslation}, +privateDecls{privateDecls} {} + + /// Creates a heap allocated struct containing space for each private + /// variable. Invariant: privateVarTypes, privateDecls, and the elements of + /// the structure should all have the same order (although privateDecls which + /// do not read from the mold argument are skipped). + void generateTaskContextStruct(); + + /// Create GEPs to access each member of the structure representing a private + /// variable, adding them to llvmPrivateVars. Null values are added where + /// private decls were skipped so that the ordering continues to match the + /// private decls. + void createGEPsToPrivateVars(); + + /// De-allocate the task context structure. + void freeStructPtr(); + + MutableArrayRef getLLVMPrivateVars() { +return llvmPrivateVars; + } + + llvm::Value *getStructPtr() { return structPtr; } + +private: + llvm::IRBuilderBase &builder; + LLVM::ModuleTranslation &moduleTranslation; + MutableArrayRef privateDecls; + + /// The type of each member of the structure, in order. + SmallVector privateVarTypes; + + /// LLVM values for each private variable, or null if that private variable is + /// not included in the task context structure + SmallVector llvmPrivateVars; bhandarkar-pranav wrote: nit: Wouldn't `llvmPrivateVarGEPs` be more descriptive? For context, I felt the need on line 1926 below ``` for (auto [privDecl, mlirPrivVar, blockArg, llvmPrivateVarAlloc] : llvm::zip_equal(privateDecls, mlirPrivateVars, privateBlockArgs, taskStructMgr.getLLVMPrivateVars())) { ``` https://github.com/llvm/llvm-project/pull/125307 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [mlir] [mlir][OpenMP] Pack task private variables into a heap-allocated context struct (PR #125307)
@@ -1794,38 +1909,114 @@ convertOmpTaskOp(omp::TaskOp taskOp, llvm::IRBuilderBase &builder, moduleTranslation, allocaIP); // Allocate and initialize private variables - // TODO: package private variables up in a structure builder.SetInsertPoint(initBlock->getTerminator()); - for (auto [privDecl, mlirPrivVar, blockArg] : - llvm::zip_equal(privateDecls, mlirPrivateVars, privateBlockArgs)) { -llvm::Type *llvmAllocType = -moduleTranslation.convertType(privDecl.getType()); -// Allocations: -builder.SetInsertPoint(allocaIP.getBlock()->getTerminator()); -llvm::Value *llvmPrivateVar = builder.CreateAlloca( -llvmAllocType, /*ArraySize=*/nullptr, "omp.private.alloc"); + // Create task variable structure + taskStructMgr.generateTaskContextStruct(); + // GEPs so that we can initialize the variables. Don't use these GEPs inside + // of the body otherwise it will be the GEP not the struct which is fowarded + // to the outlined function. GEPs forwarded in this way are passed in a + // stack-allocated (by OpenMPIRBuilder) structure which is not safe for tasks + // which may not be executed until after the current stack frame goes out of + // scope. + taskStructMgr.createGEPsToPrivateVars(); + + for (auto [privDecl, mlirPrivVar, blockArg, llvmPrivateVarAlloc] : + llvm::zip_equal(privateDecls, mlirPrivateVars, privateBlockArgs, + taskStructMgr.getLLVMPrivateVars())) { +if (!privDecl.readsFromMold()) + // to be handled inside the task + continue; +assert(llvmPrivateVarAlloc && + "reads from mold so shouldn't have been skipped"); -// builder.SetInsertPoint(initBlock->getTerminator()); -auto err = +llvm::Expected privateVarOrErr = initPrivateVar(builder, moduleTranslation, privDecl, mlirPrivVar, - blockArg, llvmPrivateVar, llvmPrivateVars, initBlock); -if (err) + blockArg, llvmPrivateVarAlloc, initBlock); +if (auto err = privateVarOrErr.takeError()) return handleError(std::move(err), *taskOp.getOperation()); + +llvm::IRBuilderBase::InsertPointGuard guard(builder); +builder.SetInsertPoint(builder.GetInsertBlock()->getTerminator()); + +// TODO: this is a bit of a hack for Fortran character boxes bhandarkar-pranav wrote: Could you please elaborate on the problem/special handling here in the comments? https://github.com/llvm/llvm-project/pull/125307 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [mlir] [mlir][OpenMP] Pack task private variables into a heap-allocated context struct (PR #125307)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/125307 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [mlir] [mlir][OpenMP] Pack task private variables into a heap-allocated context struct (PR #125307)
@@ -1730,6 +1730,126 @@ buildDependData(std::optional dependKinds, OperandRange dependVars, } } +static bool privatizerReadsSourceVariable(omp::PrivateClauseOp &priv) { + if (priv.getDataSharingType() == omp::DataSharingClauseType::FirstPrivate) +return true; + + Region &initRegion = priv.getInitRegion(); + if (initRegion.empty()) +return false; + + BlockArgument sourceVariable = priv.getInitMoldArg(); + if (!sourceVariable) +return false; + return !sourceVariable.use_empty(); +} + +namespace { +/// TaskContextStructManager takes care of creating and freeing a structure +/// containing information needed by the task body to execute. +class TaskContextStructManager { +public: + TaskContextStructManager(llvm::IRBuilderBase &builder, + LLVM::ModuleTranslation &moduleTranslation, + MutableArrayRef privateDecls) + : builder{builder}, moduleTranslation{moduleTranslation}, +privateDecls{privateDecls} {} + + /// Creates a heap allocated struct containing space for each private + /// variable. Returns nullptr if there are is no struct needed. Invariant: + /// privateVarTypes, privateDecls, and the elements of the structure should + /// all have the same order (although privateDecls which do not read from the + /// mold argument are skipped). + void generateTaskContextStruct(); + + /// Create GEPs to access each member of the structure representing a private + /// variable, adding them to llvmPrivateVars. Null values are added where + /// private decls were skipped so that the ordering continues to match the + /// private decls. + void createGEPsToPrivateVars(SmallVectorImpl &llvmPrivateVars); + + /// De-allocate the task context structure. + void freeStructPtr(); + + llvm::Value *getStructPtr() { return structPtr; } + +private: + llvm::IRBuilderBase &builder; + LLVM::ModuleTranslation &moduleTranslation; + MutableArrayRef privateDecls; + + /// The type of each member of the structure, in order. + SmallVector privateVarTypes; + + /// A pointer to the structure containing context for this task. + llvm::Value *structPtr = nullptr; + /// The type of the structure + llvm::Type *structTy = nullptr; +}; +} // namespace + +void TaskContextStructManager::generateTaskContextStruct() { + if (privateDecls.empty()) +return; + privateVarTypes.reserve(privateDecls.size()); + + for (omp::PrivateClauseOp &privOp : privateDecls) { +// Skip private variables which can safely be allocated and initialised +// inside of the task +if (!privatizerReadsSourceVariable(privOp)) + continue; +Type mlirType = privOp.getType(); +privateVarTypes.push_back(moduleTranslation.convertType(mlirType)); + } + + structTy = llvm::StructType::get(moduleTranslation.getLLVMContext(), + privateVarTypes); + + llvm::DataLayout dataLayout = + builder.GetInsertBlock()->getModule()->getDataLayout(); + llvm::Type *intPtrTy = builder.getIntPtrTy(dataLayout); + llvm::Constant *allocSize = llvm::ConstantExpr::getSizeOf(structTy); + + // Heap allocate the structure + structPtr = builder.CreateMalloc(intPtrTy, structTy, allocSize, + /*ArraySize=*/nullptr, /*MallocF=*/nullptr, + "omp.task.context_ptr"); +} + +void TaskContextStructManager::createGEPsToPrivateVars( +SmallVectorImpl &llvmPrivateVars) { + if (!structPtr) { +assert(privateVarTypes.empty()); +return; + } + + // Create GEPs for each struct member and initialize llvmPrivateVars to point bhandarkar-pranav wrote: Comment incomplete, perhaps? https://github.com/llvm/llvm-project/pull/125307 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [OpenMP][MLIR] Refactor code related to collecting privatizer info into a shared util (PR #131582)
https://github.com/bhandarkar-pranav approved this pull request. LGTM, thank you for this change. https://github.com/llvm/llvm-project/pull/131582 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Assert on map translation functions, NFC (PR #137199)
@@ -3623,6 +3623,9 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers( LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder, llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo, MapInfoData &mapData, uint64_t mapDataIndex, bool isTargetParams) { + assert(!ompBuilder.Config.isTargetDevice() && + "function only supported for host device codegen"); bhandarkar-pranav wrote: > Yes, I do agree that "host device" seems like a rather contradictory term, > since we generally just talk about "host" as the CPU and "device" as whatever > we're offloading to. The reason of using these terms is to align with OpenMP > terminology (5.2 spec, Section 1.2.1): > > > **host device** The _device_ on which the _OpenMP program_ begins execution. > > **target device** A _device_ with respect to which the current _device_ > > performs an operation, as specified by a _device construct_ or an OpenMP > > device memory routine. > > This is also why the `-fopenmp-is-target-device` flag is called that and not > `-fopenmp-is-device` (which [used to be its > name](https://reviews.llvm.org/D154591)). Thank you, good to know. So in this world every thing is a device and some devices are hosts and some are targets meant to be offloaded to. https://github.com/llvm/llvm-project/pull/137199 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Assert on map translation functions, NFC (PR #137199)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/137199 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Assert on map translation functions, NFC (PR #137199)
https://github.com/bhandarkar-pranav approved this pull request. LGTM with one (nit) comment. https://github.com/llvm/llvm-project/pull/137199 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Assert on map translation functions, NFC (PR #137199)
@@ -3623,6 +3623,9 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers( LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder, llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo, MapInfoData &mapData, uint64_t mapDataIndex, bool isTargetParams) { + assert(!ompBuilder.Config.isTargetDevice() && + "function only supported for host device codegen"); bhandarkar-pranav wrote: : The wording would be that much clearer if you didn't use the word device here at all. `"function only supported for host codegen"` or `"function only supported for host side codegen"` I know that here you the two terms you are using that are opposite to each other in meaning are "host device" and "target device", but when an assert is hit only one of them will be seen. If it is "target device" that's fine because target, target device and device are sort of used interchangeably (in my experience so far). However, I have come across ample usage of just "device" to mean target only. So, host device can be confusing. However, I do also recognize that "hostdevice" is used in other places, in at least the runtime. So, if "host device" is more prevalent, then ignore this comment entirely. https://github.com/llvm/llvm-project/pull/137199 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [MLIR][OpenMP] Assert on map translation functions, NFC (PR #137199)
@@ -3623,6 +3623,9 @@ static llvm::omp::OpenMPOffloadMappingFlags mapParentWithMembers( LLVM::ModuleTranslation &moduleTranslation, llvm::IRBuilderBase &builder, llvm::OpenMPIRBuilder &ompBuilder, DataLayout &dl, MapInfosTy &combinedInfo, MapInfoData &mapData, uint64_t mapDataIndex, bool isTargetParams) { + assert(!ompBuilder.Config.isTargetDevice() && + "function only supported for host device codegen"); bhandarkar-pranav wrote: Ok, i just saw your diagram and I see "host device" is something. I'll just wait for you to correct me at this point ;-) https://github.com/llvm/llvm-project/pull/137199 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][fir] Add `fir.local` op for locality specifiers (PR #138505)
@@ -3485,6 +3485,137 @@ def fir_BoxTotalElementsOp let hasCanonicalizer = 1; } +def YieldOp : fir_Op<"yield", +[Pure, ReturnLike, Terminator, + ParentOneOf<["LocalitySpecifierOp"]>]> { + let summary = "loop yield and termination operation"; + let description = [{ +"fir.yield" yields SSA values from the fir dialect op region and bhandarkar-pranav wrote: NIT: `sed s/from the fir dialect op region/from a fir dialect op region/` https://github.com/llvm/llvm-project/pull/138505 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][fir] Add `fir.local` op for locality specifiers (PR #138505)
https://github.com/bhandarkar-pranav approved this pull request. LGTM aside from the two nits and, more importantly, the tests that @tblah has requested https://github.com/llvm/llvm-project/pull/138505 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][fir] Add `fir.local` op for locality specifiers (PR #138505)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/138505 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [flang][fir] Add `fir.local` op for locality specifiers (PR #138505)
@@ -3485,6 +3485,137 @@ def fir_BoxTotalElementsOp let hasCanonicalizer = 1; } +def YieldOp : fir_Op<"yield", +[Pure, ReturnLike, Terminator, + ParentOneOf<["LocalitySpecifierOp"]>]> { + let summary = "loop yield and termination operation"; + let description = [{ +"fir.yield" yields SSA values from the fir dialect op region and +terminates the region. The semantics of how the values are yielded is +defined by the parent operation. + }]; + + let arguments = (ins Variadic:$results); + + let builders = [ +OpBuilder<(ins), [{ build($_builder, $_state, {}); }]> + ]; + + let assemblyFormat = "( `(` $results^ `:` type($results) `)` )? attr-dict"; +} + +def fir_LocalitySpecifierOp : fir_Op<"local", [IsolatedFromAbove]> { + let summary = "Provides declaration of local and local_init logic."; + let description = [{ +This operation provides a declaration of how to implement the +localization of a variable. The dialect users should provide +which type should be allocated for this variable. The allocated (usually by +alloca) variable is passed to the initialization region which does everything +else (e.g. initialization of Fortran runtime descriptors). Information about +how to initialize the copy from the original item should be given in the +copy region, and if needed, how to deallocate memory (allocated by the +initialization region) in the dealloc region. + +Examples: + +* `local(x)` would not need any regions because no initialization is + required by the standard for i32 variables and this is not local_init. +``` +fir.local {type = local} @x.localizer : i32 +``` + +* `local_init(x)` would be emitted as: +``` +fir.local {type = local_init} @x.localizer : i32 copy { +^bb0(%arg0: !fir.ref, %arg1: !fir.ref): +// %arg0 is the original host variable. +// %arg1 represents the memory allocated for this private variable. +... copy from host to the localized clone +fir.yield(%arg1 : !fir.ref) +} +``` + +* `local(x)` for "allocatables" would be emitted as: +``` +fir.local {type = local} @x.privatizer : !some.type init { bhandarkar-pranav wrote: Ultra Nit : `sed s/@x.privatizer/@x.localizer/` https://github.com/llvm/llvm-project/pull/138505 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/141715 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav edited https://github.com/llvm/llvm-project/pull/141715 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav created https://github.com/llvm/llvm-project/pull/141715 This PR adds functionality to the `MapInfoFinalization` pass wherein the underlying data pointer of a `fir.boxchar` is mapped as a member of the parent boxchar. >From 61cfbc7dcffc7da452dabdc8cd9b82a9b333c20a Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Fri, 23 May 2025 10:26:14 -0500 Subject: [PATCH 1/2] Fix boxchar with firstprivate --- .../Optimizer/Builder/DirectivesCommon.h | 85 +- flang/lib/Optimizer/Dialect/FIRType.cpp | 3 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 88 ++- .../OpenMP/MapsForPrivatizedSymbols.cpp | 67 -- .../Fir/convert-to-llvm-openmp-and-fir.fir| 27 ++ flang/test/Lower/OpenMP/map-character.f90 | 23 +++-- .../Lower/OpenMP/optional-argument-map-2.f90 | 63 +++-- 7 files changed, 297 insertions(+), 59 deletions(-) diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h index 3f30c761acb4e..be11b9b5ede7c 100644 --- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h +++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h @@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder, return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy); } + // For boxchar references, do the same as what is done above for box + // references - Load the boxchar so that it is easier to retrieve the length + // of the underlying character and the data pointer. + if (auto boxCharType = mlir::dyn_cast( + fir::unwrapRefType((symAddr.getType() { +if (!isOptional && mlir::isa(symAddr.getType())) { + mlir::Value boxChar = builder.create(loc, symAddr); + return AddrAndBoundsInfo(boxChar, rawInput, isPresent); +} + } return AddrAndBoundsInfo(symAddr, rawInput, isPresent); } @@ -137,26 +147,61 @@ template mlir::Value genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc, fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) { - // TODO: Handle info.isPresent. - if (auto boxCharType = - mlir::dyn_cast(info.addr.getType())) { -mlir::Type idxTy = builder.getIndexType(); -mlir::Type lenType = builder.getCharacterLengthType(); + + if (!mlir::isa(fir::unwrapRefType(info.addr.getType( +return mlir::Value{}; + + mlir::Type idxTy = builder.getIndexType(); + mlir::Type lenType = builder.getCharacterLengthType(); + mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); + mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); + using ExtentAndStride = std::tuple; + auto [extent, stride] = [&]() -> ExtentAndStride { +if (info.isPresent) { + llvm::SmallVector resTypes = {idxTy, idxTy}; + mlir::Operation::result_range ifRes = + builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true) + .genThen([&]() { +mlir::Value boxChar = +fir::isa_ref_type(info.addr.getType()) +? builder.create(loc, info.addr) +: info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); +mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); +auto unboxed = builder.create( +loc, refType, lenType, boxChar); +mlir::SmallVector results = {unboxed.getResult(1), one }; +builder.create(loc, results); + }) + .genElse([&]() { +mlir::SmallVector results = {zero, zero }; +builder.create(loc, results); }) + .getResults(); + return {ifRes[0], ifRes[1]}; +} +// We have already established that info.addr.getType() is a boxchar +// or a boxchar address. If an address, load the boxchar. +mlir::Value boxChar = fir::isa_ref_type(info.addr.getType()) + ? builder.create(loc, info.addr) + : info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); auto unboxed = -builder.create(loc, refType, lenType, info.addr); -mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); -mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); -mlir::Value extent = unboxed.getResult(1); -mlir::Value stride = one; -mlir::Value ub = builder.create(loc, extent, one); -mlir::Type boundTy = builder.getType(); -return builder.create( -loc, boundTy, /*lower_bound=*/zero, -/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride, -/*stride_in_bytes=*/true, /*start_idx=*/zero); - } - return mlir::Value{}; +builder.create(loc, refType,
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav updated https://github.com/llvm/llvm-project/pull/141715 >From 24bf7f3c48a0c4ff6755273c3917183040ccb9cb Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Fri, 23 May 2025 10:26:14 -0500 Subject: [PATCH 1/4] Fix boxchar with firstprivate --- .../Optimizer/Builder/DirectivesCommon.h | 85 +- flang/lib/Optimizer/Dialect/FIRType.cpp | 3 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 88 ++- .../OpenMP/MapsForPrivatizedSymbols.cpp | 67 -- .../Fir/convert-to-llvm-openmp-and-fir.fir| 27 ++ flang/test/Lower/OpenMP/map-character.f90 | 23 +++-- .../Lower/OpenMP/optional-argument-map-2.f90 | 63 +++-- 7 files changed, 297 insertions(+), 59 deletions(-) diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h index 3f30c761acb4e..be11b9b5ede7c 100644 --- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h +++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h @@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder, return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy); } + // For boxchar references, do the same as what is done above for box + // references - Load the boxchar so that it is easier to retrieve the length + // of the underlying character and the data pointer. + if (auto boxCharType = mlir::dyn_cast( + fir::unwrapRefType((symAddr.getType() { +if (!isOptional && mlir::isa(symAddr.getType())) { + mlir::Value boxChar = builder.create(loc, symAddr); + return AddrAndBoundsInfo(boxChar, rawInput, isPresent); +} + } return AddrAndBoundsInfo(symAddr, rawInput, isPresent); } @@ -137,26 +147,61 @@ template mlir::Value genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc, fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) { - // TODO: Handle info.isPresent. - if (auto boxCharType = - mlir::dyn_cast(info.addr.getType())) { -mlir::Type idxTy = builder.getIndexType(); -mlir::Type lenType = builder.getCharacterLengthType(); + + if (!mlir::isa(fir::unwrapRefType(info.addr.getType( +return mlir::Value{}; + + mlir::Type idxTy = builder.getIndexType(); + mlir::Type lenType = builder.getCharacterLengthType(); + mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); + mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); + using ExtentAndStride = std::tuple; + auto [extent, stride] = [&]() -> ExtentAndStride { +if (info.isPresent) { + llvm::SmallVector resTypes = {idxTy, idxTy}; + mlir::Operation::result_range ifRes = + builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true) + .genThen([&]() { +mlir::Value boxChar = +fir::isa_ref_type(info.addr.getType()) +? builder.create(loc, info.addr) +: info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); +mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); +auto unboxed = builder.create( +loc, refType, lenType, boxChar); +mlir::SmallVector results = {unboxed.getResult(1), one }; +builder.create(loc, results); + }) + .genElse([&]() { +mlir::SmallVector results = {zero, zero }; +builder.create(loc, results); }) + .getResults(); + return {ifRes[0], ifRes[1]}; +} +// We have already established that info.addr.getType() is a boxchar +// or a boxchar address. If an address, load the boxchar. +mlir::Value boxChar = fir::isa_ref_type(info.addr.getType()) + ? builder.create(loc, info.addr) + : info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); auto unboxed = -builder.create(loc, refType, lenType, info.addr); -mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); -mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); -mlir::Value extent = unboxed.getResult(1); -mlir::Value stride = one; -mlir::Value ub = builder.create(loc, extent, one); -mlir::Type boundTy = builder.getType(); -return builder.create( -loc, boundTy, /*lower_bound=*/zero, -/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride, -/*stride_in_bytes=*/true, /*start_idx=*/zero); - } - return mlir::Value{}; +builder.create(loc, refType, lenType, boxChar); +return {unboxed.getResult(1), one}; + }(); + + mlir::Value ub = builder.create(loc, extent, one); + mlir::Type boundTy = builder.getType()
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav updated https://github.com/llvm/llvm-project/pull/141715 >From bc9fbf66365fbde456813ff9c4354fb378ee45cf Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Fri, 23 May 2025 10:26:14 -0500 Subject: [PATCH 1/4] Fix boxchar with firstprivate --- .../Optimizer/Builder/DirectivesCommon.h | 85 +- flang/lib/Optimizer/Dialect/FIRType.cpp | 3 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 88 ++- .../OpenMP/MapsForPrivatizedSymbols.cpp | 67 -- .../Fir/convert-to-llvm-openmp-and-fir.fir| 27 ++ flang/test/Lower/OpenMP/map-character.f90 | 23 +++-- .../Lower/OpenMP/optional-argument-map-2.f90 | 63 +++-- 7 files changed, 297 insertions(+), 59 deletions(-) diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h index 3f30c761acb4e..be11b9b5ede7c 100644 --- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h +++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h @@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder, return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy); } + // For boxchar references, do the same as what is done above for box + // references - Load the boxchar so that it is easier to retrieve the length + // of the underlying character and the data pointer. + if (auto boxCharType = mlir::dyn_cast( + fir::unwrapRefType((symAddr.getType() { +if (!isOptional && mlir::isa(symAddr.getType())) { + mlir::Value boxChar = builder.create(loc, symAddr); + return AddrAndBoundsInfo(boxChar, rawInput, isPresent); +} + } return AddrAndBoundsInfo(symAddr, rawInput, isPresent); } @@ -137,26 +147,61 @@ template mlir::Value genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc, fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) { - // TODO: Handle info.isPresent. - if (auto boxCharType = - mlir::dyn_cast(info.addr.getType())) { -mlir::Type idxTy = builder.getIndexType(); -mlir::Type lenType = builder.getCharacterLengthType(); + + if (!mlir::isa(fir::unwrapRefType(info.addr.getType( +return mlir::Value{}; + + mlir::Type idxTy = builder.getIndexType(); + mlir::Type lenType = builder.getCharacterLengthType(); + mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); + mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); + using ExtentAndStride = std::tuple; + auto [extent, stride] = [&]() -> ExtentAndStride { +if (info.isPresent) { + llvm::SmallVector resTypes = {idxTy, idxTy}; + mlir::Operation::result_range ifRes = + builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true) + .genThen([&]() { +mlir::Value boxChar = +fir::isa_ref_type(info.addr.getType()) +? builder.create(loc, info.addr) +: info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); +mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); +auto unboxed = builder.create( +loc, refType, lenType, boxChar); +mlir::SmallVector results = {unboxed.getResult(1), one }; +builder.create(loc, results); + }) + .genElse([&]() { +mlir::SmallVector results = {zero, zero }; +builder.create(loc, results); }) + .getResults(); + return {ifRes[0], ifRes[1]}; +} +// We have already established that info.addr.getType() is a boxchar +// or a boxchar address. If an address, load the boxchar. +mlir::Value boxChar = fir::isa_ref_type(info.addr.getType()) + ? builder.create(loc, info.addr) + : info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); auto unboxed = -builder.create(loc, refType, lenType, info.addr); -mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); -mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); -mlir::Value extent = unboxed.getResult(1); -mlir::Value stride = one; -mlir::Value ub = builder.create(loc, extent, one); -mlir::Type boundTy = builder.getType(); -return builder.create( -loc, boundTy, /*lower_bound=*/zero, -/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride, -/*stride_in_bytes=*/true, /*start_idx=*/zero); - } - return mlir::Value{}; +builder.create(loc, refType, lenType, boxChar); +return {unboxed.getResult(1), one}; + }(); + + mlir::Value ub = builder.create(loc, extent, one); + mlir::Type boundTy = builder.getType()
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav updated https://github.com/llvm/llvm-project/pull/141715 >From bc9fbf66365fbde456813ff9c4354fb378ee45cf Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Fri, 23 May 2025 10:26:14 -0500 Subject: [PATCH 1/5] Fix boxchar with firstprivate --- .../Optimizer/Builder/DirectivesCommon.h | 85 +- flang/lib/Optimizer/Dialect/FIRType.cpp | 3 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 88 ++- .../OpenMP/MapsForPrivatizedSymbols.cpp | 67 -- .../Fir/convert-to-llvm-openmp-and-fir.fir| 27 ++ flang/test/Lower/OpenMP/map-character.f90 | 23 +++-- .../Lower/OpenMP/optional-argument-map-2.f90 | 63 +++-- 7 files changed, 297 insertions(+), 59 deletions(-) diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h index 3f30c761acb4e..be11b9b5ede7c 100644 --- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h +++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h @@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder, return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy); } + // For boxchar references, do the same as what is done above for box + // references - Load the boxchar so that it is easier to retrieve the length + // of the underlying character and the data pointer. + if (auto boxCharType = mlir::dyn_cast( + fir::unwrapRefType((symAddr.getType() { +if (!isOptional && mlir::isa(symAddr.getType())) { + mlir::Value boxChar = builder.create(loc, symAddr); + return AddrAndBoundsInfo(boxChar, rawInput, isPresent); +} + } return AddrAndBoundsInfo(symAddr, rawInput, isPresent); } @@ -137,26 +147,61 @@ template mlir::Value genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc, fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) { - // TODO: Handle info.isPresent. - if (auto boxCharType = - mlir::dyn_cast(info.addr.getType())) { -mlir::Type idxTy = builder.getIndexType(); -mlir::Type lenType = builder.getCharacterLengthType(); + + if (!mlir::isa(fir::unwrapRefType(info.addr.getType( +return mlir::Value{}; + + mlir::Type idxTy = builder.getIndexType(); + mlir::Type lenType = builder.getCharacterLengthType(); + mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); + mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); + using ExtentAndStride = std::tuple; + auto [extent, stride] = [&]() -> ExtentAndStride { +if (info.isPresent) { + llvm::SmallVector resTypes = {idxTy, idxTy}; + mlir::Operation::result_range ifRes = + builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true) + .genThen([&]() { +mlir::Value boxChar = +fir::isa_ref_type(info.addr.getType()) +? builder.create(loc, info.addr) +: info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); +mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); +auto unboxed = builder.create( +loc, refType, lenType, boxChar); +mlir::SmallVector results = {unboxed.getResult(1), one }; +builder.create(loc, results); + }) + .genElse([&]() { +mlir::SmallVector results = {zero, zero }; +builder.create(loc, results); }) + .getResults(); + return {ifRes[0], ifRes[1]}; +} +// We have already established that info.addr.getType() is a boxchar +// or a boxchar address. If an address, load the boxchar. +mlir::Value boxChar = fir::isa_ref_type(info.addr.getType()) + ? builder.create(loc, info.addr) + : info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); auto unboxed = -builder.create(loc, refType, lenType, info.addr); -mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); -mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); -mlir::Value extent = unboxed.getResult(1); -mlir::Value stride = one; -mlir::Value ub = builder.create(loc, extent, one); -mlir::Type boundTy = builder.getType(); -return builder.create( -loc, boundTy, /*lower_bound=*/zero, -/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride, -/*stride_in_bytes=*/true, /*start_idx=*/zero); - } - return mlir::Value{}; +builder.create(loc, refType, lenType, boxChar); +return {unboxed.getResult(1), one}; + }(); + + mlir::Value ub = builder.create(loc, extent, one); + mlir::Type boundTy = builder.getType()
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
@@ -285,6 +285,62 @@ class MapInfoFinalizationPass return false; } + mlir::omp::MapInfoOp genBoxcharMemberMap(mlir::omp::MapInfoOp op, bhandarkar-pranav wrote: Thank you, @agozillon for the review. Yes, it was related to the fact that `genDescriptorMemberMaps` is tied quite tightly to `BaseBoxType`. So, I decided to not mess with it. I should add it to my to-do list though to generalize `genDescriptorMemberMaps`. https://github.com/llvm/llvm-project/pull/141715 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav updated https://github.com/llvm/llvm-project/pull/141715 >From 24bf7f3c48a0c4ff6755273c3917183040ccb9cb Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Fri, 23 May 2025 10:26:14 -0500 Subject: [PATCH 1/3] Fix boxchar with firstprivate --- .../Optimizer/Builder/DirectivesCommon.h | 85 +- flang/lib/Optimizer/Dialect/FIRType.cpp | 3 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 88 ++- .../OpenMP/MapsForPrivatizedSymbols.cpp | 67 -- .../Fir/convert-to-llvm-openmp-and-fir.fir| 27 ++ flang/test/Lower/OpenMP/map-character.f90 | 23 +++-- .../Lower/OpenMP/optional-argument-map-2.f90 | 63 +++-- 7 files changed, 297 insertions(+), 59 deletions(-) diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h index 3f30c761acb4e..be11b9b5ede7c 100644 --- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h +++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h @@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder, return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy); } + // For boxchar references, do the same as what is done above for box + // references - Load the boxchar so that it is easier to retrieve the length + // of the underlying character and the data pointer. + if (auto boxCharType = mlir::dyn_cast( + fir::unwrapRefType((symAddr.getType() { +if (!isOptional && mlir::isa(symAddr.getType())) { + mlir::Value boxChar = builder.create(loc, symAddr); + return AddrAndBoundsInfo(boxChar, rawInput, isPresent); +} + } return AddrAndBoundsInfo(symAddr, rawInput, isPresent); } @@ -137,26 +147,61 @@ template mlir::Value genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc, fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) { - // TODO: Handle info.isPresent. - if (auto boxCharType = - mlir::dyn_cast(info.addr.getType())) { -mlir::Type idxTy = builder.getIndexType(); -mlir::Type lenType = builder.getCharacterLengthType(); + + if (!mlir::isa(fir::unwrapRefType(info.addr.getType( +return mlir::Value{}; + + mlir::Type idxTy = builder.getIndexType(); + mlir::Type lenType = builder.getCharacterLengthType(); + mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); + mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); + using ExtentAndStride = std::tuple; + auto [extent, stride] = [&]() -> ExtentAndStride { +if (info.isPresent) { + llvm::SmallVector resTypes = {idxTy, idxTy}; + mlir::Operation::result_range ifRes = + builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true) + .genThen([&]() { +mlir::Value boxChar = +fir::isa_ref_type(info.addr.getType()) +? builder.create(loc, info.addr) +: info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); +mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); +auto unboxed = builder.create( +loc, refType, lenType, boxChar); +mlir::SmallVector results = {unboxed.getResult(1), one }; +builder.create(loc, results); + }) + .genElse([&]() { +mlir::SmallVector results = {zero, zero }; +builder.create(loc, results); }) + .getResults(); + return {ifRes[0], ifRes[1]}; +} +// We have already established that info.addr.getType() is a boxchar +// or a boxchar address. If an address, load the boxchar. +mlir::Value boxChar = fir::isa_ref_type(info.addr.getType()) + ? builder.create(loc, info.addr) + : info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); auto unboxed = -builder.create(loc, refType, lenType, info.addr); -mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); -mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); -mlir::Value extent = unboxed.getResult(1); -mlir::Value stride = one; -mlir::Value ub = builder.create(loc, extent, one); -mlir::Type boundTy = builder.getType(); -return builder.create( -loc, boundTy, /*lower_bound=*/zero, -/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride, -/*stride_in_bytes=*/true, /*start_idx=*/zero); - } - return mlir::Value{}; +builder.create(loc, refType, lenType, boxChar); +return {unboxed.getResult(1), one}; + }(); + + mlir::Value ub = builder.create(loc, extent, one); + mlir::Type boundTy = builder.getType()
[llvm-branch-commits] [flang] [Flang][OpenMP] - When mapping a `fir.boxchar`, map the underlying data pointer as a member (PR #141715)
https://github.com/bhandarkar-pranav updated https://github.com/llvm/llvm-project/pull/141715 >From 2d411fc5d24c7e3e933447307fc958b7e544490b Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Fri, 23 May 2025 10:26:14 -0500 Subject: [PATCH 1/5] Fix boxchar with firstprivate --- .../Optimizer/Builder/DirectivesCommon.h | 85 +- flang/lib/Optimizer/Dialect/FIRType.cpp | 3 + .../Optimizer/OpenMP/MapInfoFinalization.cpp | 88 ++- .../OpenMP/MapsForPrivatizedSymbols.cpp | 67 -- .../Fir/convert-to-llvm-openmp-and-fir.fir| 27 ++ flang/test/Lower/OpenMP/map-character.f90 | 23 +++-- .../Lower/OpenMP/optional-argument-map-2.f90 | 63 +++-- 7 files changed, 297 insertions(+), 59 deletions(-) diff --git a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h index 3f30c761acb4e..be11b9b5ede7c 100644 --- a/flang/include/flang/Optimizer/Builder/DirectivesCommon.h +++ b/flang/include/flang/Optimizer/Builder/DirectivesCommon.h @@ -91,6 +91,16 @@ inline AddrAndBoundsInfo getDataOperandBaseAddr(fir::FirOpBuilder &builder, return AddrAndBoundsInfo(symAddr, rawInput, isPresent, boxTy); } + // For boxchar references, do the same as what is done above for box + // references - Load the boxchar so that it is easier to retrieve the length + // of the underlying character and the data pointer. + if (auto boxCharType = mlir::dyn_cast( + fir::unwrapRefType((symAddr.getType() { +if (!isOptional && mlir::isa(symAddr.getType())) { + mlir::Value boxChar = builder.create(loc, symAddr); + return AddrAndBoundsInfo(boxChar, rawInput, isPresent); +} + } return AddrAndBoundsInfo(symAddr, rawInput, isPresent); } @@ -137,26 +147,61 @@ template mlir::Value genBoundsOpFromBoxChar(fir::FirOpBuilder &builder, mlir::Location loc, fir::ExtendedValue dataExv, AddrAndBoundsInfo &info) { - // TODO: Handle info.isPresent. - if (auto boxCharType = - mlir::dyn_cast(info.addr.getType())) { -mlir::Type idxTy = builder.getIndexType(); -mlir::Type lenType = builder.getCharacterLengthType(); + + if (!mlir::isa(fir::unwrapRefType(info.addr.getType( +return mlir::Value{}; + + mlir::Type idxTy = builder.getIndexType(); + mlir::Type lenType = builder.getCharacterLengthType(); + mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); + mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); + using ExtentAndStride = std::tuple; + auto [extent, stride] = [&]() -> ExtentAndStride { +if (info.isPresent) { + llvm::SmallVector resTypes = {idxTy, idxTy}; + mlir::Operation::result_range ifRes = + builder.genIfOp(loc, resTypes, info.isPresent, /*withElseRegion=*/true) + .genThen([&]() { +mlir::Value boxChar = +fir::isa_ref_type(info.addr.getType()) +? builder.create(loc, info.addr) +: info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); +mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); +auto unboxed = builder.create( +loc, refType, lenType, boxChar); +mlir::SmallVector results = {unboxed.getResult(1), one }; +builder.create(loc, results); + }) + .genElse([&]() { +mlir::SmallVector results = {zero, zero }; +builder.create(loc, results); }) + .getResults(); + return {ifRes[0], ifRes[1]}; +} +// We have already established that info.addr.getType() is a boxchar +// or a boxchar address. If an address, load the boxchar. +mlir::Value boxChar = fir::isa_ref_type(info.addr.getType()) + ? builder.create(loc, info.addr) + : info.addr; +fir::BoxCharType boxCharType = +mlir::cast(boxChar.getType()); mlir::Type refType = builder.getRefType(boxCharType.getEleTy()); auto unboxed = -builder.create(loc, refType, lenType, info.addr); -mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0); -mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1); -mlir::Value extent = unboxed.getResult(1); -mlir::Value stride = one; -mlir::Value ub = builder.create(loc, extent, one); -mlir::Type boundTy = builder.getType(); -return builder.create( -loc, boundTy, /*lower_bound=*/zero, -/*upper_bound=*/ub, /*extent=*/extent, /*stride=*/stride, -/*stride_in_bytes=*/true, /*start_idx=*/zero); - } - return mlir::Value{}; +builder.create(loc, refType, lenType, boxChar); +return {unboxed.getResult(1), one}; + }(); + + mlir::Value ub = builder.create(loc, extent, one); + mlir::Type boundTy = builder.getType()