[mlir][acc] Improve acc.loop support as a container #137886
Closed
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
added 4 commits
April 29, 2025 09:27
The acc.loop printer inserted two spaces after the operation. This occurred because the custom combined loop attribute printer was not conditional - and thus the tablegen inserted an automatic space before invoking the custom printer. Then for each additional attribute it also inserted a space in beginning. Since lit tests were not sensitive to this, no tests need updated. But the issue with the extraneous space is resolved.
Dialects which have their own loop representation not representable with numeric bounds + steps cannot be represented cleanly with acc.loop. In such a case, we permit the dialects representation with acc.loop merely encompasing its loop representation. This limitation became obvious when looking at range / random iterator C++ loops. The API of acc.loop was updated to test for this differentiation. Additionally, the verifier was updated to check for consistent bounds and whether inner-loops are contained when it works as a container.
Member
|
@llvm/pr-subscribers-mlir-openacc Author: Razvan Lupusoru (razvanlupusoru) ChangesDialects which have their own loop representation not representable The API of acc.loop was updated to test for this differentiation. Full diff: https://github.com/llvm/llvm-project/pull/137886.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 41cec89fdf598..3ad8e4f9ccbeb 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2127,6 +2127,11 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
/// Used to retrieve the block inside the op's region.
Block &getBody() { return getLoopRegions().front()->front(); }
+ /// Used to determine if this operation is merely a container for a loop
+ /// operation instead of being loop-like itself.
+ bool isLoopLike() { return !getLowerbound().empty(); }
+ bool isContainerLike() { return !isLoopLike(); }
+
/// Return true if the op has the auto attribute for the
/// mlir::acc::DeviceType::None device_type.
bool hasAuto();
@@ -2197,7 +2202,7 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
let hasCustomAssemblyFormat = 1;
let assemblyFormat = [{
- custom<CombinedConstructsLoop>($combined)
+ ( `combined` `(` custom<CombinedConstructsLoop>($combined)^ `)` )?
oilist(
`gang` `` custom<GangClause>($gangOperands, type($gangOperands),
$gangOperandsArgType, $gangOperandsDeviceType,
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index c4cc560e42f6a..a7b01abe34e03 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -1686,25 +1686,19 @@ static void printDeviceTypeOperandsWithKeywordOnly(
static ParseResult
parseCombinedConstructsLoop(mlir::OpAsmParser &parser,
mlir::acc::CombinedConstructsTypeAttr &attr) {
- if (succeeded(parser.parseOptionalKeyword("combined"))) {
- if (parser.parseLParen())
- return failure();
- if (succeeded(parser.parseOptionalKeyword("kernels"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::KernelsLoop);
- } else if (succeeded(parser.parseOptionalKeyword("parallel"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::ParallelLoop);
- } else if (succeeded(parser.parseOptionalKeyword("serial"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::SerialLoop);
- } else {
- parser.emitError(parser.getCurrentLocation(),
- "expected compute construct name");
- return failure();
- }
- if (parser.parseRParen())
- return failure();
+ if (succeeded(parser.parseOptionalKeyword("kernels"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::KernelsLoop);
+ } else if (succeeded(parser.parseOptionalKeyword("parallel"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::ParallelLoop);
+ } else if (succeeded(parser.parseOptionalKeyword("serial"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::SerialLoop);
+ } else {
+ parser.emitError(parser.getCurrentLocation(),
+ "expected compute construct name");
+ return failure();
}
return success();
}
@@ -1715,13 +1709,13 @@ printCombinedConstructsLoop(mlir::OpAsmPrinter &p, mlir::Operation *op,
if (attr) {
switch (attr.getValue()) {
case mlir::acc::CombinedConstructsType::KernelsLoop:
- p << "combined(kernels)";
+ p << "kernels";
break;
case mlir::acc::CombinedConstructsType::ParallelLoop:
- p << "combined(parallel)";
+ p << "parallel";
break;
case mlir::acc::CombinedConstructsType::SerialLoop:
- p << "combined(serial)";
+ p << "serial";
break;
};
}
@@ -2304,6 +2298,14 @@ LogicalResult checkDeviceTypes(mlir::ArrayAttr deviceTypes) {
}
LogicalResult acc::LoopOp::verify() {
+ if (getUpperbound().size() != getStep().size())
+ return emitError() << "number of upperbounds expected to be the same as "
+ "number of steps";
+
+ if (getUpperbound().size() != getLowerbound().size())
+ return emitError() << "number of upperbounds expected to be the same as "
+ "number of lowerbounds";
+
if (!getUpperbound().empty() && getInclusiveUpperbound() &&
(getUpperbound().size() != getInclusiveUpperbound()->size()))
return emitError() << "inclusiveUpperbound size is expected to be the same"
@@ -2421,6 +2423,15 @@ LogicalResult acc::LoopOp::verify() {
if (getRegion().empty())
return emitError("expected non-empty body.");
+ // When it is container-like - it is expected to hold a loop-like operation.
+ // TODO: Get the collapse attribute into account.
+ if (isContainerLike()) {
+ // TODO: Ensure there is a single loop-like operation at any one level.
+ auto loopLikeOps = getRegion().getOps<LoopLikeOpInterface>();
+ if (loopLikeOps.empty())
+ return emitError("expected to hold a loop-like operation.");
+ }
+
return success();
}
|
Member
|
@llvm/pr-subscribers-openacc Author: Razvan Lupusoru (razvanlupusoru) ChangesDialects which have their own loop representation not representable The API of acc.loop was updated to test for this differentiation. Full diff: https://github.com/llvm/llvm-project/pull/137886.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 41cec89fdf598..3ad8e4f9ccbeb 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2127,6 +2127,11 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
/// Used to retrieve the block inside the op's region.
Block &getBody() { return getLoopRegions().front()->front(); }
+ /// Used to determine if this operation is merely a container for a loop
+ /// operation instead of being loop-like itself.
+ bool isLoopLike() { return !getLowerbound().empty(); }
+ bool isContainerLike() { return !isLoopLike(); }
+
/// Return true if the op has the auto attribute for the
/// mlir::acc::DeviceType::None device_type.
bool hasAuto();
@@ -2197,7 +2202,7 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
let hasCustomAssemblyFormat = 1;
let assemblyFormat = [{
- custom<CombinedConstructsLoop>($combined)
+ ( `combined` `(` custom<CombinedConstructsLoop>($combined)^ `)` )?
oilist(
`gang` `` custom<GangClause>($gangOperands, type($gangOperands),
$gangOperandsArgType, $gangOperandsDeviceType,
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index c4cc560e42f6a..a7b01abe34e03 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -1686,25 +1686,19 @@ static void printDeviceTypeOperandsWithKeywordOnly(
static ParseResult
parseCombinedConstructsLoop(mlir::OpAsmParser &parser,
mlir::acc::CombinedConstructsTypeAttr &attr) {
- if (succeeded(parser.parseOptionalKeyword("combined"))) {
- if (parser.parseLParen())
- return failure();
- if (succeeded(parser.parseOptionalKeyword("kernels"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::KernelsLoop);
- } else if (succeeded(parser.parseOptionalKeyword("parallel"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::ParallelLoop);
- } else if (succeeded(parser.parseOptionalKeyword("serial"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::SerialLoop);
- } else {
- parser.emitError(parser.getCurrentLocation(),
- "expected compute construct name");
- return failure();
- }
- if (parser.parseRParen())
- return failure();
+ if (succeeded(parser.parseOptionalKeyword("kernels"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::KernelsLoop);
+ } else if (succeeded(parser.parseOptionalKeyword("parallel"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::ParallelLoop);
+ } else if (succeeded(parser.parseOptionalKeyword("serial"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::SerialLoop);
+ } else {
+ parser.emitError(parser.getCurrentLocation(),
+ "expected compute construct name");
+ return failure();
}
return success();
}
@@ -1715,13 +1709,13 @@ printCombinedConstructsLoop(mlir::OpAsmPrinter &p, mlir::Operation *op,
if (attr) {
switch (attr.getValue()) {
case mlir::acc::CombinedConstructsType::KernelsLoop:
- p << "combined(kernels)";
+ p << "kernels";
break;
case mlir::acc::CombinedConstructsType::ParallelLoop:
- p << "combined(parallel)";
+ p << "parallel";
break;
case mlir::acc::CombinedConstructsType::SerialLoop:
- p << "combined(serial)";
+ p << "serial";
break;
};
}
@@ -2304,6 +2298,14 @@ LogicalResult checkDeviceTypes(mlir::ArrayAttr deviceTypes) {
}
LogicalResult acc::LoopOp::verify() {
+ if (getUpperbound().size() != getStep().size())
+ return emitError() << "number of upperbounds expected to be the same as "
+ "number of steps";
+
+ if (getUpperbound().size() != getLowerbound().size())
+ return emitError() << "number of upperbounds expected to be the same as "
+ "number of lowerbounds";
+
if (!getUpperbound().empty() && getInclusiveUpperbound() &&
(getUpperbound().size() != getInclusiveUpperbound()->size()))
return emitError() << "inclusiveUpperbound size is expected to be the same"
@@ -2421,6 +2423,15 @@ LogicalResult acc::LoopOp::verify() {
if (getRegion().empty())
return emitError("expected non-empty body.");
+ // When it is container-like - it is expected to hold a loop-like operation.
+ // TODO: Get the collapse attribute into account.
+ if (isContainerLike()) {
+ // TODO: Ensure there is a single loop-like operation at any one level.
+ auto loopLikeOps = getRegion().getOps<LoopLikeOpInterface>();
+ if (loopLikeOps.empty())
+ return emitError("expected to hold a loop-like operation.");
+ }
+
return success();
}
|
Member
|
@llvm/pr-subscribers-mlir Author: Razvan Lupusoru (razvanlupusoru) ChangesDialects which have their own loop representation not representable The API of acc.loop was updated to test for this differentiation. Full diff: https://github.com/llvm/llvm-project/pull/137886.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 41cec89fdf598..3ad8e4f9ccbeb 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2127,6 +2127,11 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
/// Used to retrieve the block inside the op's region.
Block &getBody() { return getLoopRegions().front()->front(); }
+ /// Used to determine if this operation is merely a container for a loop
+ /// operation instead of being loop-like itself.
+ bool isLoopLike() { return !getLowerbound().empty(); }
+ bool isContainerLike() { return !isLoopLike(); }
+
/// Return true if the op has the auto attribute for the
/// mlir::acc::DeviceType::None device_type.
bool hasAuto();
@@ -2197,7 +2202,7 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
let hasCustomAssemblyFormat = 1;
let assemblyFormat = [{
- custom<CombinedConstructsLoop>($combined)
+ ( `combined` `(` custom<CombinedConstructsLoop>($combined)^ `)` )?
oilist(
`gang` `` custom<GangClause>($gangOperands, type($gangOperands),
$gangOperandsArgType, $gangOperandsDeviceType,
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index c4cc560e42f6a..a7b01abe34e03 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -1686,25 +1686,19 @@ static void printDeviceTypeOperandsWithKeywordOnly(
static ParseResult
parseCombinedConstructsLoop(mlir::OpAsmParser &parser,
mlir::acc::CombinedConstructsTypeAttr &attr) {
- if (succeeded(parser.parseOptionalKeyword("combined"))) {
- if (parser.parseLParen())
- return failure();
- if (succeeded(parser.parseOptionalKeyword("kernels"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::KernelsLoop);
- } else if (succeeded(parser.parseOptionalKeyword("parallel"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::ParallelLoop);
- } else if (succeeded(parser.parseOptionalKeyword("serial"))) {
- attr = mlir::acc::CombinedConstructsTypeAttr::get(
- parser.getContext(), mlir::acc::CombinedConstructsType::SerialLoop);
- } else {
- parser.emitError(parser.getCurrentLocation(),
- "expected compute construct name");
- return failure();
- }
- if (parser.parseRParen())
- return failure();
+ if (succeeded(parser.parseOptionalKeyword("kernels"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::KernelsLoop);
+ } else if (succeeded(parser.parseOptionalKeyword("parallel"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::ParallelLoop);
+ } else if (succeeded(parser.parseOptionalKeyword("serial"))) {
+ attr = mlir::acc::CombinedConstructsTypeAttr::get(
+ parser.getContext(), mlir::acc::CombinedConstructsType::SerialLoop);
+ } else {
+ parser.emitError(parser.getCurrentLocation(),
+ "expected compute construct name");
+ return failure();
}
return success();
}
@@ -1715,13 +1709,13 @@ printCombinedConstructsLoop(mlir::OpAsmPrinter &p, mlir::Operation *op,
if (attr) {
switch (attr.getValue()) {
case mlir::acc::CombinedConstructsType::KernelsLoop:
- p << "combined(kernels)";
+ p << "kernels";
break;
case mlir::acc::CombinedConstructsType::ParallelLoop:
- p << "combined(parallel)";
+ p << "parallel";
break;
case mlir::acc::CombinedConstructsType::SerialLoop:
- p << "combined(serial)";
+ p << "serial";
break;
};
}
@@ -2304,6 +2298,14 @@ LogicalResult checkDeviceTypes(mlir::ArrayAttr deviceTypes) {
}
LogicalResult acc::LoopOp::verify() {
+ if (getUpperbound().size() != getStep().size())
+ return emitError() << "number of upperbounds expected to be the same as "
+ "number of steps";
+
+ if (getUpperbound().size() != getLowerbound().size())
+ return emitError() << "number of upperbounds expected to be the same as "
+ "number of lowerbounds";
+
if (!getUpperbound().empty() && getInclusiveUpperbound() &&
(getUpperbound().size() != getInclusiveUpperbound()->size()))
return emitError() << "inclusiveUpperbound size is expected to be the same"
@@ -2421,6 +2423,15 @@ LogicalResult acc::LoopOp::verify() {
if (getRegion().empty())
return emitError("expected non-empty body.");
+ // When it is container-like - it is expected to hold a loop-like operation.
+ // TODO: Get the collapse attribute into account.
+ if (isContainerLike()) {
+ // TODO: Ensure there is a single loop-like operation at any one level.
+ auto loopLikeOps = getRegion().getOps<LoopLikeOpInterface>();
+ if (loopLikeOps.empty())
+ return emitError("expected to hold a loop-like operation.");
+ }
+
return success();
}
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Dialects which have their own loop representation not representable
with numeric bounds + steps cannot be represented cleanly with
acc.loop. In such a case, we permit the dialects representation with
acc.loop merely encompasing its loop representation. This limitation
became obvious when looking at range / random iterator C++ loops.
The API of acc.loop was updated to test for this differentiation.
Additionally, the verifier was updated to check for consistent bounds
and whether inner-loops are contained when it works as a container.