Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it up to the caller to ensure recipe with that name have not been created yet/what happens if it already exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.).

::mlir::Value var,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have or envision use cases for having var provided as an mlir::Value instead of just passing its type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

::llvm::StringRef varName = "",
::mlir::ValueRange bounds = {});
}];
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -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 = {});
}];
}

//===----------------------------------------------------------------------===//
Expand Down
44 changes: 32 additions & 12 deletions mlir/include/mlir/Dialect/OpenACC/OpenACCTypeInterfaces.td
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -94,31 +102,43 @@ 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 {};
}]
>,
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=*/[{
Expand Down
Loading