[MLIR][OpenMP] Remove the ReductionClauseInterface, NFC #130978
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
This patch removes the `ReductionClauseInterface` and all definitions of its associated `getAllReductionVars` method. The method mandated by this interface is not used anywhere and the conflicts its definition produces when multiple reduction clauses are present in an operation result in a more convoluted operation definition, so it seems better to remove it and only add something like this if there's a clear advantage to it.
skatrak
requested review from
ergawy,
Meinersbur,
jsjodin,
tblah,
kiranchandramohan,
NimishMishra and
kiranktp
|
@llvm/pr-subscribers-flang-openmp Author: Sergio Afonso (skatrak) ChangesThis patch removes the The method mandated by this interface is not used anywhere and the conflicts its definition produces when multiple reduction clauses are present in an operation result in a more convoluted operation definition, so it seems better to remove it and only add something like this if there's a clear advantage to it. Full diff: https://github.com/llvm/llvm-project/pull/130978.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
index 12da584926af8..f8e880ea43b75 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
@@ -606,7 +606,7 @@ class OpenMP_InReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -615,14 +615,6 @@ class OpenMP_InReductionClauseSkip<
OptionalAttr<SymbolRefArrayAttr>:$in_reduction_syms
);
- let extraClassDeclaration = [{
- /// Returns the reduction variables.
- SmallVector<Value> getAllReductionVars() {
- return SmallVector<Value>(getInReductionVars().begin(),
- getInReductionVars().end());
- }
- }];
-
// Description varies depending on the operation. Assembly format not defined
// because this clause must be processed together with the first region of the
// operation, as it defines entry block arguments.
@@ -1156,7 +1148,7 @@ class OpenMP_ReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -1287,7 +1279,7 @@ class OpenMP_TaskReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -1296,14 +1288,6 @@ class OpenMP_TaskReductionClauseSkip<
OptionalAttr<SymbolRefArrayAttr>:$task_reduction_syms
);
- let extraClassDeclaration = [{
- /// Returns the reduction variables.
- SmallVector<Value> getAllReductionVars() {
- return SmallVector<Value>(getTaskReductionVars().begin(),
- getTaskReductionVars().end());
- }
- }];
-
let description = [{
The `task_reduction` clause specifies a reduction among tasks. For each list
item, the number of copies is unspecified. Any copies associated with the
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index 401c4c11d8986..5752446494280 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -751,11 +751,9 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
RecursiveMemoryEffects, SingleBlock
], clauses = [
OpenMP_AllocateClause, OpenMP_FinalClause, OpenMP_GrainsizeClause,
- OpenMP_IfClause, OpenMP_InReductionClauseSkip<extraClassDeclaration = true>,
- OpenMP_MergeableClause, OpenMP_NogroupClause, OpenMP_NumTasksClause,
- OpenMP_PriorityClause, OpenMP_PrivateClause,
- OpenMP_ReductionClauseSkip<extraClassDeclaration = true>,
- OpenMP_UntiedClause
+ OpenMP_IfClause, OpenMP_InReductionClause, OpenMP_MergeableClause,
+ OpenMP_NogroupClause, OpenMP_NumTasksClause, OpenMP_PriorityClause,
+ OpenMP_PrivateClause, OpenMP_ReductionClause, OpenMP_UntiedClause
], singleRegion = true> {
let summary = "taskloop construct";
let description = [{
@@ -821,9 +819,6 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
}];
let extraClassDeclaration = [{
- /// Returns the reduction variables
- SmallVector<Value> getAllReductionVars();
-
void getEffects(SmallVectorImpl<MemoryEffects::EffectInstance> &effects);
}] # clausesExtraClassDeclaration;
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
index 0766b4e8d1472..614412737b538 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
@@ -212,22 +212,6 @@ def MapClauseOwningOpInterface : OpInterface<"MapClauseOwningOpInterface"> {
];
}
-def ReductionClauseInterface : OpInterface<"ReductionClauseInterface"> {
- let description = [{
- OpenMP operations that support reduction clause have this interface.
- }];
-
- let cppNamespace = "::mlir::omp";
-
- let methods = [
- InterfaceMethod<
- "Get reduction vars", "::mlir::SmallVector<::mlir::Value>",
- "getAllReductionVars", (ins), [{}], [{
- return $_op.getReductionVars();
- }]>,
- ];
-}
-
def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
let description = [{
OpenMP operations that wrap a single loop nest. They must only contain a
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 84b4d30076646..7b0df1281fef8 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2726,14 +2726,6 @@ void TaskloopOp::build(OpBuilder &builder, OperationState &state,
makeArrayAttr(ctx, clauses.reductionSyms), clauses.untied);
}
-SmallVector<Value> TaskloopOp::getAllReductionVars() {
- SmallVector<Value> allReductionNvars(getInReductionVars().begin(),
- getInReductionVars().end());
- allReductionNvars.insert(allReductionNvars.end(), getReductionVars().begin(),
- getReductionVars().end());
- return allReductionNvars;
-}
-
LogicalResult TaskloopOp::verify() {
if (getAllocateVars().size() != getAllocatorVars().size())
return emitError(
|
|
@llvm/pr-subscribers-mlir Author: Sergio Afonso (skatrak) ChangesThis patch removes the The method mandated by this interface is not used anywhere and the conflicts its definition produces when multiple reduction clauses are present in an operation result in a more convoluted operation definition, so it seems better to remove it and only add something like this if there's a clear advantage to it. Full diff: https://github.com/llvm/llvm-project/pull/130978.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
index 12da584926af8..f8e880ea43b75 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
@@ -606,7 +606,7 @@ class OpenMP_InReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -615,14 +615,6 @@ class OpenMP_InReductionClauseSkip<
OptionalAttr<SymbolRefArrayAttr>:$in_reduction_syms
);
- let extraClassDeclaration = [{
- /// Returns the reduction variables.
- SmallVector<Value> getAllReductionVars() {
- return SmallVector<Value>(getInReductionVars().begin(),
- getInReductionVars().end());
- }
- }];
-
// Description varies depending on the operation. Assembly format not defined
// because this clause must be processed together with the first region of the
// operation, as it defines entry block arguments.
@@ -1156,7 +1148,7 @@ class OpenMP_ReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -1287,7 +1279,7 @@ class OpenMP_TaskReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -1296,14 +1288,6 @@ class OpenMP_TaskReductionClauseSkip<
OptionalAttr<SymbolRefArrayAttr>:$task_reduction_syms
);
- let extraClassDeclaration = [{
- /// Returns the reduction variables.
- SmallVector<Value> getAllReductionVars() {
- return SmallVector<Value>(getTaskReductionVars().begin(),
- getTaskReductionVars().end());
- }
- }];
-
let description = [{
The `task_reduction` clause specifies a reduction among tasks. For each list
item, the number of copies is unspecified. Any copies associated with the
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index 401c4c11d8986..5752446494280 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -751,11 +751,9 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
RecursiveMemoryEffects, SingleBlock
], clauses = [
OpenMP_AllocateClause, OpenMP_FinalClause, OpenMP_GrainsizeClause,
- OpenMP_IfClause, OpenMP_InReductionClauseSkip<extraClassDeclaration = true>,
- OpenMP_MergeableClause, OpenMP_NogroupClause, OpenMP_NumTasksClause,
- OpenMP_PriorityClause, OpenMP_PrivateClause,
- OpenMP_ReductionClauseSkip<extraClassDeclaration = true>,
- OpenMP_UntiedClause
+ OpenMP_IfClause, OpenMP_InReductionClause, OpenMP_MergeableClause,
+ OpenMP_NogroupClause, OpenMP_NumTasksClause, OpenMP_PriorityClause,
+ OpenMP_PrivateClause, OpenMP_ReductionClause, OpenMP_UntiedClause
], singleRegion = true> {
let summary = "taskloop construct";
let description = [{
@@ -821,9 +819,6 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
}];
let extraClassDeclaration = [{
- /// Returns the reduction variables
- SmallVector<Value> getAllReductionVars();
-
void getEffects(SmallVectorImpl<MemoryEffects::EffectInstance> &effects);
}] # clausesExtraClassDeclaration;
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
index 0766b4e8d1472..614412737b538 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
@@ -212,22 +212,6 @@ def MapClauseOwningOpInterface : OpInterface<"MapClauseOwningOpInterface"> {
];
}
-def ReductionClauseInterface : OpInterface<"ReductionClauseInterface"> {
- let description = [{
- OpenMP operations that support reduction clause have this interface.
- }];
-
- let cppNamespace = "::mlir::omp";
-
- let methods = [
- InterfaceMethod<
- "Get reduction vars", "::mlir::SmallVector<::mlir::Value>",
- "getAllReductionVars", (ins), [{}], [{
- return $_op.getReductionVars();
- }]>,
- ];
-}
-
def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
let description = [{
OpenMP operations that wrap a single loop nest. They must only contain a
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 84b4d30076646..7b0df1281fef8 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2726,14 +2726,6 @@ void TaskloopOp::build(OpBuilder &builder, OperationState &state,
makeArrayAttr(ctx, clauses.reductionSyms), clauses.untied);
}
-SmallVector<Value> TaskloopOp::getAllReductionVars() {
- SmallVector<Value> allReductionNvars(getInReductionVars().begin(),
- getInReductionVars().end());
- allReductionNvars.insert(allReductionNvars.end(), getReductionVars().begin(),
- getReductionVars().end());
- return allReductionNvars;
-}
-
LogicalResult TaskloopOp::verify() {
if (getAllocateVars().size() != getAllocatorVars().size())
return emitError(
|
|
@llvm/pr-subscribers-mlir-openmp Author: Sergio Afonso (skatrak) ChangesThis patch removes the The method mandated by this interface is not used anywhere and the conflicts its definition produces when multiple reduction clauses are present in an operation result in a more convoluted operation definition, so it seems better to remove it and only add something like this if there's a clear advantage to it. Full diff: https://github.com/llvm/llvm-project/pull/130978.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
index 12da584926af8..f8e880ea43b75 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td
@@ -606,7 +606,7 @@ class OpenMP_InReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -615,14 +615,6 @@ class OpenMP_InReductionClauseSkip<
OptionalAttr<SymbolRefArrayAttr>:$in_reduction_syms
);
- let extraClassDeclaration = [{
- /// Returns the reduction variables.
- SmallVector<Value> getAllReductionVars() {
- return SmallVector<Value>(getInReductionVars().begin(),
- getInReductionVars().end());
- }
- }];
-
// Description varies depending on the operation. Assembly format not defined
// because this clause must be processed together with the first region of the
// operation, as it defines entry block arguments.
@@ -1156,7 +1148,7 @@ class OpenMP_ReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -1287,7 +1279,7 @@ class OpenMP_TaskReductionClauseSkip<
> : OpenMP_Clause<traits, arguments, assemblyFormat, description,
extraClassDeclaration> {
let traits = [
- BlockArgOpenMPOpInterface, ReductionClauseInterface
+ BlockArgOpenMPOpInterface
];
let arguments = (ins
@@ -1296,14 +1288,6 @@ class OpenMP_TaskReductionClauseSkip<
OptionalAttr<SymbolRefArrayAttr>:$task_reduction_syms
);
- let extraClassDeclaration = [{
- /// Returns the reduction variables.
- SmallVector<Value> getAllReductionVars() {
- return SmallVector<Value>(getTaskReductionVars().begin(),
- getTaskReductionVars().end());
- }
- }];
-
let description = [{
The `task_reduction` clause specifies a reduction among tasks. For each list
item, the number of copies is unspecified. Any copies associated with the
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index 401c4c11d8986..5752446494280 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -751,11 +751,9 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
RecursiveMemoryEffects, SingleBlock
], clauses = [
OpenMP_AllocateClause, OpenMP_FinalClause, OpenMP_GrainsizeClause,
- OpenMP_IfClause, OpenMP_InReductionClauseSkip<extraClassDeclaration = true>,
- OpenMP_MergeableClause, OpenMP_NogroupClause, OpenMP_NumTasksClause,
- OpenMP_PriorityClause, OpenMP_PrivateClause,
- OpenMP_ReductionClauseSkip<extraClassDeclaration = true>,
- OpenMP_UntiedClause
+ OpenMP_IfClause, OpenMP_InReductionClause, OpenMP_MergeableClause,
+ OpenMP_NogroupClause, OpenMP_NumTasksClause, OpenMP_PriorityClause,
+ OpenMP_PrivateClause, OpenMP_ReductionClause, OpenMP_UntiedClause
], singleRegion = true> {
let summary = "taskloop construct";
let description = [{
@@ -821,9 +819,6 @@ def TaskloopOp : OpenMP_Op<"taskloop", traits = [
}];
let extraClassDeclaration = [{
- /// Returns the reduction variables
- SmallVector<Value> getAllReductionVars();
-
void getEffects(SmallVectorImpl<MemoryEffects::EffectInstance> &effects);
}] # clausesExtraClassDeclaration;
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
index 0766b4e8d1472..614412737b538 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsInterfaces.td
@@ -212,22 +212,6 @@ def MapClauseOwningOpInterface : OpInterface<"MapClauseOwningOpInterface"> {
];
}
-def ReductionClauseInterface : OpInterface<"ReductionClauseInterface"> {
- let description = [{
- OpenMP operations that support reduction clause have this interface.
- }];
-
- let cppNamespace = "::mlir::omp";
-
- let methods = [
- InterfaceMethod<
- "Get reduction vars", "::mlir::SmallVector<::mlir::Value>",
- "getAllReductionVars", (ins), [{}], [{
- return $_op.getReductionVars();
- }]>,
- ];
-}
-
def LoopWrapperInterface : OpInterface<"LoopWrapperInterface"> {
let description = [{
OpenMP operations that wrap a single loop nest. They must only contain a
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 84b4d30076646..7b0df1281fef8 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -2726,14 +2726,6 @@ void TaskloopOp::build(OpBuilder &builder, OperationState &state,
makeArrayAttr(ctx, clauses.reductionSyms), clauses.untied);
}
-SmallVector<Value> TaskloopOp::getAllReductionVars() {
- SmallVector<Value> allReductionNvars(getInReductionVars().begin(),
- getInReductionVars().end());
- allReductionNvars.insert(allReductionNvars.end(), getReductionVars().begin(),
- getReductionVars().end());
- return allReductionNvars;
-}
-
LogicalResult TaskloopOp::verify() {
if (getAllocateVars().size() != getAllocatorVars().size())
return emitError(
|
ergawy
approved these changes
Mar 13, 2025
frederik-h
pushed a commit
to frederik-h/llvm-project
that referenced
this pull request
Mar 18, 2025
This patch removes the `ReductionClauseInterface` and all definitions of its associated `getAllReductionVars` method. The method mandated by this interface is not used anywhere and the conflicts its definition produces when multiple reduction clauses are present in an operation result in a more convoluted operation definition, so it seems better to remove it and only add something like this if there's a clear advantage to it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This patch removes the
ReductionClauseInterfaceand all definitions of its associatedgetAllReductionVarsmethod.The method mandated by this interface is not used anywhere and the conflicts its definition produces when multiple reduction clauses are present in an operation result in a more convoluted operation definition, so it seems better to remove it and only add something like this if there's a clear advantage to it.