[Mlir-commits] [mlir] [mlir][acc] Improve acc.loop support as a container (PR #137886)
Razvan Lupusoru
llvmlistbot at llvm.org
Tue Apr 29 15:19:26 PDT 2025
https://github.com/razvanlupusoru created https://github.com/llvm/llvm-project/pull/137886
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.
>From f118b6c2c818863918085ceaf1d1e5e73794cb33 Mon Sep 17 00:00:00 2001
From: Razvan Lupusoru <rlupusoru at nvidia.com>
Date: Tue, 29 Apr 2025 09:27:20 -0700
Subject: [PATCH 1/3] [mlir][acc] Fix extraneous space when printing acc.loop
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.
---
.../mlir/Dialect/OpenACC/OpenACCOps.td | 4 +-
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp | 38 ++++++++-----------
2 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 41cec89fdf598..28003e99aba5c 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2197,9 +2197,9 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
let hasCustomAssemblyFormat = 1;
let assemblyFormat = [{
- custom<CombinedConstructsLoop>($combined)
oilist(
- `gang` `` custom<GangClause>($gangOperands, type($gangOperands),
+ `combined` `(` custom<CombinedConstructsLoop>($combined) `)`
+ | `gang` `` custom<GangClause>($gangOperands, type($gangOperands),
$gangOperandsArgType, $gangOperandsDeviceType,
$gangOperandsSegments, $gang)
| `worker` `` custom<DeviceTypeOperandsWithKeywordOnly>(
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index c4cc560e42f6a..91025e90b8e76 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;
};
}
>From 0c3e331f621bfc7cdaacbd28b0337aaccb9d4acf Mon Sep 17 00:00:00 2001
From: Razvan Lupusoru <rlupusoru at nvidia.com>
Date: Tue, 29 Apr 2025 13:33:04 -0700
Subject: [PATCH 2/3] Ensure that combined appears first on acc.loop
---
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 28003e99aba5c..df7ff28ca5926 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2197,9 +2197,9 @@ def OpenACC_LoopOp : OpenACC_Op<"loop",
let hasCustomAssemblyFormat = 1;
let assemblyFormat = [{
+ ( `combined` `(` custom<CombinedConstructsLoop>($combined)^ `)` )?
oilist(
- `combined` `(` custom<CombinedConstructsLoop>($combined) `)`
- | `gang` `` custom<GangClause>($gangOperands, type($gangOperands),
+ `gang` `` custom<GangClause>($gangOperands, type($gangOperands),
$gangOperandsArgType, $gangOperandsDeviceType,
$gangOperandsSegments, $gang)
| `worker` `` custom<DeviceTypeOperandsWithKeywordOnly>(
>From 52e738cfd37dc33e7a47c2f8e76a77a9dd2607f1 Mon Sep 17 00:00:00 2001
From: Razvan Lupusoru <rlupusoru at nvidia.com>
Date: Tue, 29 Apr 2025 15:18:06 -0700
Subject: [PATCH 3/3] [mlir][acc] Improve acc.loop support as a container
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.
---
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td | 5 +++++
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp | 17 +++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index df7ff28ca5926..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();
diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
index 91025e90b8e76..a7b01abe34e03 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
@@ -2298,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"
@@ -2415,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();
}
More information about the Mlir-commits
mailing list