[mlir][acc] Introduce createAndPopulate for recipe creation #162917
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Private and firstprivate recipes can now be created and populated through the createAndPopulate method. The populating of recipe bodies is done through using the PointerLikeType and MappableType interfaces (with MappableType support still in progress). The existing create() API remains available for cases where dialects need manual recipe population (e.g., for frontend-specific semantics like default value initialization or constructor calls). Testing exercises the new API with memref types.
|
@llvm/pr-subscribers-openacc @llvm/pr-subscribers-mlir Author: Razvan Lupusoru (razvanlupusoru) ChangesPrivate and firstprivate recipes can now be created and populated through the createAndPopulate method. The populating of recipe bodies is done through using the PointerLikeType and MappableType interfaces (with MappableType support still in progress). The existing create() API remains available for cases where dialects need manual recipe population (e.g., for frontend-specific semantics like default value initialization or constructor calls). Testing exercises the new API with memref types. Patch is 33.08 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/162917.diff 9 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 77e833f8f9492..c22c0dec6a2a1 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -1316,6 +1316,22 @@ def OpenACC_PrivateRecipeOp
}];
let hasRegionVerifier = 1;
+
+ let extraClassDeclaration = [{
+ /// Creates a PrivateRecipeOp and populates its regions based on the
+ /// variable type as long as the type implements MappableType or
+ /// PointerLikeType interface. If a type implements both, the MappableType
+ /// API will be preferred. Returns std::nullopt if the recipe cannot be
+ /// created or populated. The builder's current insertion point will be used
+ /// and it must be a valid place for this operation to be inserted.
+ static std::optional<PrivateRecipeOp> createAndPopulate(
+ ::mlir::OpBuilder &builder,
+ ::mlir::Location loc,
+ ::llvm::StringRef recipeName,
+ ::mlir::Value var,
+ ::llvm::StringRef varName = "",
+ ::mlir::ValueRange bounds = {});
+ }];
}
//===----------------------------------------------------------------------===//
@@ -1410,6 +1426,22 @@ def OpenACC_FirstprivateRecipeOp
}];
let hasRegionVerifier = 1;
+
+ let extraClassDeclaration = [{
+ /// Creates a FirstprivateRecipeOp and populates its regions based on the
+ /// variable type as long as the type implements MappableType or
+ /// PointerLikeType interface. If a type implements both, the MappableType
+ /// API will be preferred. Returns std::nullopt if the recipe cannot be
+ /// created or populated. The builder's current insertion point will be used
+ /// and it must be a valid place for this operation to be inserted.
+ static std::optional<FirstprivateRecipeOp> createAndPopulate(
+ ::mlir::OpBuilder &builder,
+ ::mlir::Location loc,
+ ::llvm::StringRef recipeName,
+ ::mlir::Value var,
+ ::llvm::StringRef varName = "",
+ ::mlir::ValueRange bounds = {});
+ }];
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td
index 0d16255c5a994..684cf70b5c045 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td
@@ -83,7 +83,15 @@ def OpenACC_PointerLikeTypeInterface : TypeInterface<"PointerLikeType"> {
The `originalVar` parameter is optional but enables support for dynamic
types (e.g., dynamic memrefs). When provided, implementations can extract
runtime dimension information from the original variable to create
- allocations with matching dynamic sizes.
+ allocations with matching dynamic sizes. When generating recipe bodies,
+ `originalVar` should be the block argument representing the original
+ variable in the recipe region.
+
+ The `needsFree` output parameter indicates whether the allocated memory
+ requires explicit deallocation. Implementations should set this to true
+ for heap allocations that need a matching deallocation operation (e.g.,
+ alloc) and false for stack-based allocations (e.g., alloca). During
+ recipe generation, this determines whether a destroy region is created.
Returns a Value representing the result of the allocation. If no value
is returned, it means the allocation was not successfully generated.
@@ -94,7 +102,8 @@ def OpenACC_PointerLikeTypeInterface : TypeInterface<"PointerLikeType"> {
"::mlir::Location":$loc,
"::llvm::StringRef":$varName,
"::mlir::Type":$varType,
- "::mlir::Value":$originalVar),
+ "::mlir::Value":$originalVar,
+ "bool &":$needsFree),
/*methodBody=*/"",
/*defaultImplementation=*/[{
return {};
@@ -102,23 +111,34 @@ def OpenACC_PointerLikeTypeInterface : TypeInterface<"PointerLikeType"> {
>,
InterfaceMethod<
/*description=*/[{
- Generates deallocation operations for the pointer-like type. It deallocates
- the instance provided.
+ Generates deallocation operations for the pointer-like type.
- The `varPtr` parameter is required and must represent an instance that was
- previously allocated. If the current type is represented in a way that it
- does not capture the pointee type, `varType` must be passed in to provide
- the necessary type information. Nothing is generated in case the allocate
- is `alloca`-like.
+ The `varToFree` parameter is required and must represent an instance
+ that was previously allocated. When generating recipe bodies, this
+ should be the block argument representing the private variable in the
+ destroy region.
+
+ The `allocRes` parameter is optional and provides the result of the
+ corresponding allocation from the init region. This allows implementations
+ to inspect the allocation operation to determine the appropriate
+ deallocation strategy. This is necessary because in recipe generation,
+ the allocation and deallocation occur in separate regions. Dialects that
+ use only one allocation type or can determine deallocation from type
+ information alone may ignore this parameter.
+
+ The `varType` parameter must be provided if the current type does not
+ capture the pointee type information. No deallocation is generated for
+ stack-based allocations (e.g., alloca).
- Returns true if deallocation was successfully generated or successfully
- deemed as not needed to be generated, false otherwise.
+ Returns true if deallocation was successfully generated or determined to
+ be unnecessary, false otherwise.
}],
/*retTy=*/"bool",
/*methodName=*/"genFree",
/*args=*/(ins "::mlir::OpBuilder &":$builder,
"::mlir::Location":$loc,
- "::mlir::TypedValue<::mlir::acc::PointerLikeType>":$varPtr,
+ "::mlir::TypedValue<::mlir::acc::PointerLikeType>":$varToFree,
+ "::mlir::Value":$allocRes,
"::mlir::Type":$varType),
/*methodBody=*/"",
/*defaultImplementation=*/[{
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index 6564a4ecdccd3..be9f9342f7f18 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -74,14 +74,16 @@ struct MemRefPointerLikeModel
}
mlir::Value genAllocate(Type pointer, OpBuilder &builder, Location loc,
- StringRef varName, Type varType,
- Value originalVar) const {
+ StringRef varName, Type varType, Value originalVar,
+ bool &needsFree) const {
auto memrefTy = cast<MemRefType>(pointer);
// Check if this is a static memref (all dimensions are known) - if yes
// then we can generate an alloca operation.
- if (memrefTy.hasStaticShape())
+ if (memrefTy.hasStaticShape()) {
+ needsFree = false; // alloca doesn't need deallocation
return memref::AllocaOp::create(builder, loc, memrefTy).getResult();
+ }
// For dynamic memrefs, extract sizes from the original variable if
// provided. Otherwise they cannot be handled.
@@ -99,6 +101,7 @@ struct MemRefPointerLikeModel
// Note: We only add dynamic sizes to the dynamicSizes array
// Static dimensions are handled automatically by AllocOp
}
+ needsFree = true; // alloc needs deallocation
return memref::AllocOp::create(builder, loc, memrefTy, dynamicSizes)
.getResult();
}
@@ -108,10 +111,14 @@ struct MemRefPointerLikeModel
}
bool genFree(Type pointer, OpBuilder &builder, Location loc,
- TypedValue<PointerLikeType> varPtr, Type varType) const {
- if (auto memrefValue = dyn_cast<TypedValue<MemRefType>>(varPtr)) {
+ TypedValue<PointerLikeType> varToFree, Value allocRes,
+ Type varType) const {
+ if (auto memrefValue = dyn_cast<TypedValue<MemRefType>>(varToFree)) {
+ // Use allocRes if provided to determine the allocation type
+ Value valueToInspect = allocRes ? allocRes : memrefValue;
+
// Walk through casts to find the original allocation
- Value currentValue = memrefValue;
+ Value currentValue = valueToInspect;
Operation *originalAlloc = nullptr;
// Follow the chain of operations to find the original allocation
@@ -150,7 +157,7 @@ struct MemRefPointerLikeModel
return true;
}
if (isa<memref::AllocOp>(originalAlloc)) {
- // This is an alloc - generate dealloc
+ // This is an alloc - generate dealloc on varToFree
memref::DeallocOp::create(builder, loc, memrefValue);
return true;
}
@@ -1003,6 +1010,151 @@ struct RemoveConstantIfConditionWithRegion : public OpRewritePattern<OpTy> {
}
};
+//===----------------------------------------------------------------------===//
+// Recipe Region Helpers
+//===----------------------------------------------------------------------===//
+
+/// Create and populate an init region for privatization recipes.
+/// Returns the init block on success, or nullptr on failure.
+/// Sets needsFree to indicate if the allocated memory requires deallocation.
+static std::unique_ptr<Block> createInitRegion(OpBuilder &builder, Location loc,
+ Value var, StringRef varName,
+ ValueRange bounds, Type varType,
+ bool &needsFree) {
+ // Create init block with arguments: original value + bounds
+ SmallVector<Type> argTypes{varType};
+ SmallVector<Location> argLocs{loc};
+ for (Value bound : bounds) {
+ argTypes.push_back(bound.getType());
+ argLocs.push_back(loc);
+ }
+
+ auto initBlock = std::make_unique<Block>();
+ initBlock->addArguments(argTypes, argLocs);
+ builder.setInsertionPointToStart(initBlock.get());
+
+ Value privatizedValue;
+
+ // Get the block argument that represents the original variable
+ Value blockArgVar = initBlock->getArgument(0);
+
+ // Generate init region body based on variable type
+ if (isa<MappableType>(varType)) {
+ auto mappableTy = cast<MappableType>(varType);
+ auto typedVar = cast<TypedValue<MappableType>>(blockArgVar);
+ privatizedValue = mappableTy.generatePrivateInit(builder, loc, typedVar,
+ varName, bounds, {});
+ if (!privatizedValue) {
+ return nullptr;
+ }
+ // TODO: MappableType doesn't yet support needsFree
+ // For now assume it doesn't need explicit deallocation
+ needsFree = false;
+ } else {
+ assert(isa<PointerLikeType>(varType) && "Expected PointerLikeType");
+ auto pointerLikeTy = cast<PointerLikeType>(varType);
+ // Use PointerLikeType's allocation API with the block argument
+ privatizedValue = pointerLikeTy.genAllocate(builder, loc, varName, varType,
+ blockArgVar, needsFree);
+ if (!privatizedValue) {
+ return nullptr;
+ }
+ }
+
+ // Add yield operation to init block
+ acc::YieldOp::create(builder, loc, privatizedValue);
+
+ return initBlock;
+}
+
+/// Create and populate a copy region for firstprivate recipes.
+/// Returns the copy block on success, or nullptr on failure.
+/// TODO: Handle MappableType - it does not yet have a copy API.
+static std::unique_ptr<Block> createCopyRegion(OpBuilder &builder, Location loc,
+ ValueRange bounds,
+ Type varType) {
+ // Create copy block with arguments: original value + privatized value +
+ // bounds
+ SmallVector<Type> copyArgTypes{varType, varType};
+ SmallVector<Location> copyArgLocs{loc, loc};
+ for (Value bound : bounds) {
+ copyArgTypes.push_back(bound.getType());
+ copyArgLocs.push_back(loc);
+ }
+
+ auto copyBlock = std::make_unique<Block>();
+ copyBlock->addArguments(copyArgTypes, copyArgLocs);
+ builder.setInsertionPointToStart(copyBlock.get());
+
+ bool isMappable = isa<MappableType>(varType);
+ bool isPointerLike = isa<PointerLikeType>(varType);
+ if (isMappable && !isPointerLike) {
+ // TODO: Handle MappableType - it does not yet have a copy API.
+ // Otherwise, for now just fallback to pointer-like behavior.
+ return nullptr;
+ }
+
+ // Generate copy region body based on variable type
+ if (isPointerLike) {
+ auto pointerLikeTy = cast<PointerLikeType>(varType);
+ Value originalArg = copyBlock->getArgument(0);
+ Value privatizedArg = copyBlock->getArgument(1);
+
+ // Generate copy operation using PointerLikeType interface
+ if (!pointerLikeTy.genCopy(
+ builder, loc, cast<TypedValue<PointerLikeType>>(privatizedArg),
+ cast<TypedValue<PointerLikeType>>(originalArg), varType)) {
+ return nullptr;
+ }
+ }
+
+ // Add terminator to copy block
+ acc::TerminatorOp::create(builder, loc);
+
+ return copyBlock;
+}
+
+/// Create and populate a destroy region for privatization recipes.
+/// Returns the destroy block on success, or nullptr if not needed.
+static std::unique_ptr<Block> createDestroyRegion(OpBuilder &builder,
+ Location loc, Value allocRes,
+ ValueRange bounds,
+ Type varType) {
+ // Create destroy block with arguments: original value + privatized value +
+ // bounds
+ SmallVector<Type> destroyArgTypes{varType, varType};
+ SmallVector<Location> destroyArgLocs{loc, loc};
+ for (Value bound : bounds) {
+ destroyArgTypes.push_back(bound.getType());
+ destroyArgLocs.push_back(loc);
+ }
+
+ auto destroyBlock = std::make_unique<Block>();
+ destroyBlock->addArguments(destroyArgTypes, destroyArgLocs);
+ builder.setInsertionPointToStart(destroyBlock.get());
+
+ bool isMappable = isa<MappableType>(varType);
+ bool isPointerLike = isa<PointerLikeType>(varType);
+ if (isMappable && !isPointerLike) {
+ // TODO: Handle MappableType - it does not yet have a deallocation API.
+ // Otherwise, for now just fallback to pointer-like behavior.
+ return nullptr;
+ }
+
+ assert(isa<PointerLikeType>(varType) && "Expected PointerLikeType");
+ auto pointerLikeTy = cast<PointerLikeType>(varType);
+ auto privatizedArg =
+ cast<TypedValue<PointerLikeType>>(destroyBlock->getArgument(1));
+ // Pass allocRes to help determine the allocation type
+ if (!pointerLikeTy.genFree(builder, loc, privatizedArg, allocRes, varType)) {
+ return nullptr;
+ }
+
+ acc::TerminatorOp::create(builder, loc);
+
+ return destroyBlock;
+}
+
} // namespace
//===----------------------------------------------------------------------===//
@@ -1050,6 +1202,61 @@ LogicalResult acc::PrivateRecipeOp::verifyRegions() {
return success();
}
+std::optional<PrivateRecipeOp>
+PrivateRecipeOp::createAndPopulate(OpBuilder &builder, Location loc,
+ StringRef recipeName, Value var,
+ StringRef varName, ValueRange bounds) {
+
+ Type varType = var.getType();
+
+ // First, validate that we can handle this variable type
+ bool isMappable = isa<MappableType>(varType);
+ bool isPointerLike = isa<PointerLikeType>(varType);
+
+ if (!isMappable && !isPointerLike) {
+ // Unsupported type
+ return std::nullopt;
+ }
+
+ // Create init and destroy blocks using shared helpers
+ OpBuilder::InsertionGuard guard(builder);
+
+ // Save the original insertion point for creating the recipe operation later
+ auto originalInsertionPoint = builder.saveInsertionPoint();
+
+ bool needsFree = false;
+ auto initBlock =
+ createInitRegion(builder, loc, var, varName, bounds, varType, needsFree);
+ if (!initBlock) {
+ return std::nullopt;
+ }
+
+ // Only create destroy region if the allocation needs deallocation
+ std::unique_ptr<Block> destroyBlock;
+ if (needsFree) {
+ // Extract the allocated value from the init block's yield operation
+ auto yieldOp = cast<acc::YieldOp>(initBlock->getTerminator());
+ Value allocRes = yieldOp.getOperand(0);
+
+ destroyBlock = createDestroyRegion(builder, loc, allocRes, bounds, varType);
+ if (!destroyBlock) {
+ return std::nullopt;
+ }
+ }
+
+ // Now create the recipe operation at the original insertion point and attach
+ // the blocks
+ builder.restoreInsertionPoint(originalInsertionPoint);
+ auto recipe = PrivateRecipeOp::create(builder, loc, recipeName, varType);
+
+ // Move the blocks into the recipe's regions
+ recipe.getInitRegion().push_back(initBlock.release());
+ if (destroyBlock)
+ recipe.getDestroyRegion().push_back(destroyBlock.release());
+
+ return recipe;
+}
+
//===----------------------------------------------------------------------===//
// FirstprivateRecipeOp
//===----------------------------------------------------------------------===//
@@ -1080,6 +1287,67 @@ LogicalResult acc::FirstprivateRecipeOp::verifyRegions() {
return success();
}
+std::optional<FirstprivateRecipeOp>
+FirstprivateRecipeOp::createAndPopulate(OpBuilder &builder, Location loc,
+ StringRef recipeName, Value var,
+ StringRef varName, ValueRange bounds) {
+
+ Type varType = var.getType();
+
+ // First, validate that we can handle this variable type
+ bool isMappable = isa<MappableType>(varType);
+ bool isPointerLike = isa<PointerLikeType>(varType);
+
+ if (!isMappable && !isPointerLike) {
+ // Unsupported type
+ return std::nullopt;
+ }
+
+ // Create init, copy, and destroy blocks using shared helpers
+ OpBuilder::InsertionGuard guard(builder);
+
+ // Save the original insertion point for creating the recipe operation later
+ auto originalInsertionPoint = builder.saveInsertionPoint();
+
+ bool needsFree = false;
+ auto initBlock =
+ createInitRegion(builder, loc, var, varName, bounds, varType, needsFree);
+ if (!initBlock) {
+ return std::nullopt;
+ }
+
+ auto copyBlock = createCopyRegion(builder, loc, bounds, varType);
+ if (!copyBlock) {
+ return std::nullopt;
+ }
+
+ // Only create destroy region if the allocation needs deallocation
+ std::unique_ptr<Block> destroyBlock;
+ if (needsFree) {
+ // Extract the allocated value from the init block's yield operation
+ auto yieldOp = cast<acc::YieldOp>(initBlock->getTerminator());
+ Value allocRes = yieldOp.getOperand(0);
+
+ destroyBlock = createDestroyRegion(builder, loc, allocRes, bounds, varType);
+ if (!destroyBlock) {
+ return std::nullopt;
+ }
+ }
+
+ // Now create the recipe operation at the original insertion point and attach
+ // the blocks
+ builder.restoreInsertionPoint(originalInsertionPoint);
+ auto recipe = FirstprivateRecipeOp::create(builder, loc, recipeName, varType);
+
+ // Move the blocks into the recipe's regions
+ recipe.getInitRegion().push_back(initBlock.release());
+ recipe.getCopyRegion().push_back(copyBlock.release());
+ if (destroyBlock)
+ recipe.getDestroyRegion().push_back(destroyBlock.release());
+
+ return recipe;
+}
+
//===----------------------------------------------------------------------===//
// ReductionRecipeOp
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/OpenACC/recipe-populate-firstprivate.mlir b/mlir/test/Dialect/OpenACC/recipe-populate-firstprivate.mlir
new file mode 100644
index 0000000000000..35355c6e06164
--- /dev/null
+++ b/mlir/test/Dialect/OpenACC/recipe-populate-firstprivate.mlir
@@ -0,0 +1,102 @@
+// RUN: mlir-opt %s --split-input-file --pass-pipeline="builtin.module(test-acc-recipe-populate{recipe-type=firstprivate})" | FileCheck %s
+
+// CHECK: acc.firstprivate.recipe @firstprivate_scalar : memref<f32> init {
+// C...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
clementval
reviewed
Oct 11, 2025
jeanPerier
reviewed
Oct 13, 2025
| ::mlir::OpBuilder &builder, | ||
| ::mlir::Location loc, | ||
| ::llvm::StringRef recipeName, | ||
| ::mlir::Value var, |
There was a problem hiding this comment.
Do you have or envision use cases for having var provided as an mlir::Value instead of just passing its type?
There was a problem hiding this comment.
Good question! I actually had this internal debate myself when designing this API. The reason I went with passing var as an mlir::Value instead of just the type is to handle cases where MLIR's representation (due to lack of support for dependent types) requires the IR to "specialize" based on the actual value.
A concrete example of this design decision in practice can be seen in the generatePrivateInit implementation for MappableType in FIR (in FIROpenACCTypeInterfaces.cpp, around line 619-620):
hlfir::Entity source = hlfir::Entity{var};
auto [temp, cleanup] = hlfir::createTempFromMold(loc, firBuilder, source);Here, the value is used to create a mold via createTempFromMold. This operation needs the actual value (not just its type). With only the type information, we wouldn't be able to handle these cases where the type system doesn't fully encode all the necessary information.
Additionally, since we can always extract the type from the value (var.getType()), passing the value gives us maximum flexibility without losing any information. I imagined that all privatization decision are done in the context of an existing value. Does this reasoning make sense, or do you see issues with this approach?
There was a problem hiding this comment.
Does this reasoning make sense, or do you see issues with this approach?
The issue I see is the risk of using this value in a utility that would create an operation inside the recipe using the value while the value does not belong to the recipe which is isolated. In the FIR example that you are giving, var is the block argument of the init region, so it is not the direct equivalent of var here.
As far as FIR is concerned, the types are enough info to generate init/copy/destroy. However, some info about the actual var obtained via use/def chain lookup can lead to more optimal code (mainly contiguity info). However, it is only OK to rely on this Value info if the recipe are generated in unique fashion for each Value and not for each type (FIR OpenACC lowering re-use recipe based on the type currently).
So your interface is more powerful than just using the Type, but it comes with slightly more misuse risks.
There was a problem hiding this comment.
but it comes with slightly more misuse risks
You convinced me - createAndPopulate should not permit this misuse without stronger use case since anyway it needs to feed block arguments to MappableType/PointerLikeType APIs to avoid generating code that uses incorrect ssa value. That said, I still think that the MappableType/PointerLikeType APIs should still have access to the original variable since those may be generated inline without the use of recipes.
| static std::optional<PrivateRecipeOp> createAndPopulate( | ||
| ::mlir::OpBuilder &builder, | ||
| ::mlir::Location loc, | ||
| ::llvm::StringRef recipeName, |
There was a problem hiding this comment.
Is it up to the caller to ensure recipe with that name have not been created yet/what happens if it already exists?
There was a problem hiding this comment.
Great question! The code wasn't handling this case previously.
Contrary to my initial belief, MLIR does not automatically make symbol names unique. Instead, it allows construction of IR that will later fail verification with a "redefinition of symbol" error.
I've updated createAndPopulate to return std::nullopt when a symbol with the requested name already exists. This indicates to the caller that the operation cannot be created.
I considered two alternative approaches:
- Return the existing recipe - I rejected this because we cannot guarantee that the existing recipe is semantically equivalent to what the caller intends to create without additional validation.
- Auto-generate a unique name - I rejected this because it would silently create a recipe with a different name than what the caller requested, which could lead to unexpected behavior.
By returning std::nullopt, the caller receives clear feedback that the requested name conflicts with an existing symbol and can handle it appropriately (e.g., use a different name, check if the existing recipe is suitable, etc.).
jeanPerier
approved these changes
Oct 14, 2025
| // If it does - don't assume that we can just reuse the existing one. | ||
| Operation *parentOp = builder.getInsertionBlock()->getParentOp(); | ||
| if (parentOp && | ||
| SymbolTable::lookupNearestSymbolFrom( |
There was a problem hiding this comment.
I am pondering if you need to thread a SymbolTable* to allow for fast lookups (the lookup you have is linear with the number of symbol defined in the module, so if createAndPopulate was to be called a lot (>1000 times), that would be a compilation perf issue), but I am thinking that in practice most caller caller would likely lookup for the recipe in the module before trying to create it, so createAndPopulate should be called once per type being privatized, and I do not imagine that being more than a 100 (probably often less).
It should be up to the caller to use a SymbolTable depending on the context (we are likely not doing it in FIR OpenACC lowering right now, and we just may if we find compilation perf issues there).
So I think that is OK. Just writing this to let you double check.
There was a problem hiding this comment.
Great point Jean!
As I thought about the idea of threading through the SymbolTable to this API, it already suggests that the caller is doing some SymbolTable handling. Thus it seems of little value to push this responsibility to the API to make sure it is unique - thus I restored previous code and updated the API description to note that a unique name must be passed in.
akadutta
pushed a commit
to akadutta/llvm-project
that referenced
this pull request
Oct 14, 2025
) Private and firstprivate recipes can now be created and populated through the createAndPopulate method. The populating of recipe bodies is done through using the PointerLikeType and MappableType interfaces (with MappableType support still in progress). The existing create() API remains available for cases where dialects need manual recipe population (e.g., for frontend-specific semantics like default value initialization or constructor calls). Testing exercises the new API with memref types.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Private and firstprivate recipes can now be created and populated through the createAndPopulate method. The populating of recipe bodies is done through using the PointerLikeType and MappableType interfaces (with MappableType support still in progress).
The existing create() API remains available for cases where dialects need manual recipe population (e.g., for frontend-specific semantics like default value initialization or constructor calls).
Testing exercises the new API with memref types.