[Mlir-commits] [mlir] 08ba06e - [mlir][OpenACC] Fix tile clause element bounds for descending loops (#227185)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Tue Sep 29 08:54:07 PDT 2026
Author: Vijay Kandiah
Date: 2026-09-29T10:54:01-05:00
New Revision: 08ba06eaa556554c8afb52dad73ff70eb4bf60ec
URL: https://github.com/llvm/llvm-project/commit/08ba06eaa556554c8afb52dad73ff70eb4bf60ec
DIFF: https://github.com/llvm/llvm-project/commit/08ba06eaa556554c8afb52dad73ff70eb4bf60ec.diff
LOG: [mlir][OpenACC] Fix tile clause element bounds for descending loops (#227185)
acc.loops could have negative steps, but ACCLoopTiling computed each
element loop's bound assuming an ascending one. Both halves of that
computation were wrong for a descending dimension: the inclusive edge
subtracted one instead of adding one, and the bound was clamped with min
instead of max.
For `DO j = n, floor, -1` under `tile(32)`, the last tile's bound came
out as `min(floor, j - 33)`, which selects `j - 33` whenever it falls
below the floor. The element loop then ran past the end of the iteration
space, reading and writing outside the array.
With this change, we pick the clamp and the inclusive edge from the
step's direction. When the sign is only known at runtime, we compute
both and select on it.
The tiling tests had no coverage of negative steps or of inclusive upper
bounds. This patch adds four new tests: descending, descending with an
inclusive bound, ascending with an inclusive bound, and a runtime-signed
step.
Added:
Modified:
mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsTiling.h
mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
Removed:
################################################################################
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsTiling.h b/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsTiling.h
index 05ac37d1106ec..b24ab07c37c5c 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsTiling.h
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsTiling.h
@@ -58,6 +58,10 @@ namespace acc {
/// }
/// \endcode
///
+/// The element bounds above assume ascending steps. For a descending dimension,
+/// the tile's far edge lies below the original bound, so the clamp uses max
+/// instead of min.
+///
/// Unknown tile sizes (represented as -1 in acc dialect for `tile(*)`) are
/// resolved to the provided default tile size.
///
diff --git a/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp b/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
index d5865bc98727e..766d50fe54348 100644
--- a/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
+++ b/mlir/lib/Dialect/OpenACC/Transforms/ACCLoopTiling.cpp
@@ -35,6 +35,10 @@
// }
// }
//
+// The example element loop bounds above assume ascending steps. For an
+// acc.loop with negative steps, a descending dimension would use
+// max(ub, %i+s*T) instead.
+//
// Gang/worker/vector attributes are distributed as follows:
// - gang: applied to tile loops
// - vector: applied to element loops
diff --git a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
index 32fd3957894fb..24242f86dd4f2 100644
--- a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
+++ b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsTiling.cpp
@@ -196,7 +196,10 @@ mlir::acc::tileACCLoops(mlir::acc::LoopOp tileLoop,
}
}
- // Compute the element-loop upper bounds min(origUB, origIV + scaledStep).
+ // Compute each element-loop upper bound by clamping the tile's far edge,
+ // origIV + scaledStep, to origUB. An ascending loop takes
+ // min(origUB, edge - 1), a descending one max(origUB, edge + 1). The -1 and
+ // +1 apply in the presence of inclusive bounds and are omitted otherwise.
rewriter.setInsertionPoint(tileLoop.getBody().getTerminator());
llvm::SmallVector<mlir::Value, 3> elemLBs, elemUBs, elemSteps;
llvm::SmallVector<mlir::Type, 3> elemIVTypes;
@@ -204,16 +207,43 @@ mlir::acc::tileACCLoops(mlir::acc::LoopOp tileLoop,
for (unsigned i = 0; i < tileCount; ++i) {
mlir::Value stepped =
mlir::arith::AddIOp::create(rewriter, loc, origIVs[i], scaledSteps[i]);
- mlir::Value newUB = stepped;
- if (inclusiveUBs[i]) {
- // Inclusive UB: min(origUB, origIV + (scaledStep - 1)).
+ mlir::Type stepTy = scaledSteps[i].getType();
+ std::optional<int64_t> constStep = mlir::getConstantIntValue(origSteps[i]);
+
+ // The inclusive edge is one iteration short of the next tile's start, in
+ // whichever direction the loop runs.
+ auto inclusiveEdge = [&](bool descending) -> mlir::Value {
+ if (!inclusiveUBs[i])
+ return stepped;
mlir::Value c1 = mlir::arith::ConstantOp::create(
- rewriter, loc, scaledSteps[i].getType(),
- rewriter.getIntegerAttr(scaledSteps[i].getType(), 1));
- newUB = mlir::arith::SubIOp::create(rewriter, loc, stepped, c1);
+ rewriter, loc, stepTy, rewriter.getIntegerAttr(stepTy, 1));
+ if (descending)
+ return mlir::arith::AddIOp::create(rewriter, loc, stepped, c1);
+ return mlir::arith::SubIOp::create(rewriter, loc, stepped, c1);
+ };
+
+ mlir::Value elemUB;
+ if (constStep && *constStep < 0) {
+ elemUB = mlir::arith::MaxSIOp::create(rewriter, loc, origUBs[i],
+ inclusiveEdge(/*descending=*/true));
+ } else if (constStep) {
+ elemUB = mlir::arith::MinSIOp::create(
+ rewriter, loc, origUBs[i], inclusiveEdge(/*descending=*/false));
+ } else {
+ // The sign is only known at run time, so compute both and select.
+ mlir::Type origStepTy = origSteps[i].getType();
+ mlir::Value zero = mlir::arith::ConstantOp::create(
+ rewriter, loc, origStepTy, rewriter.getIntegerAttr(origStepTy, 0));
+ mlir::Value isDescending = mlir::arith::CmpIOp::create(
+ rewriter, loc, mlir::arith::CmpIPredicate::slt, origSteps[i], zero);
+ mlir::Value descUB = mlir::arith::MaxSIOp::create(
+ rewriter, loc, origUBs[i], inclusiveEdge(/*descending=*/true));
+ mlir::Value ascUB = mlir::arith::MinSIOp::create(
+ rewriter, loc, origUBs[i], inclusiveEdge(/*descending=*/false));
+ elemUB = mlir::arith::SelectOp::create(rewriter, loc, isDescending,
+ descUB, ascUB);
}
- elemUBs.push_back(
- mlir::arith::MinSIOp::create(rewriter, loc, origUBs[i], newUB));
+ elemUBs.push_back(elemUB);
elemLBs.push_back(origIVs[i]);
elemSteps.push_back(origSteps[i]);
elemIVTypes.push_back(origIVs[i].getType());
diff --git a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
index ad23945df7687..6db2060124e9a 100644
--- a/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
+++ b/mlir/test/Dialect/OpenACC/acc-loop-tiling.mlir
@@ -216,3 +216,91 @@ func.func @body_with_nested_region(%arg0: memref<10xi32>) {
} independent
return
}
+
+// Test a descending loop: the element bound clamps the tile's far edge up to
+// the original bound, so it uses max rather than min.
+
+// CHECK-LABEL: func.func @descending_loop_tile
+// CHECK: %[[CM4:.*]] = arith.constant -4 : index
+// CHECK: acc.loop control(%[[IV:.*]] : index) = (%{{.*}} : index) to (%[[UB:.*]] : index) step (%[[CM4]] : index) {
+// CHECK: %[[EDGE:.*]] = arith.addi %[[IV]], %[[CM4]] : index
+// CHECK: %[[MAX_UB:.*]] = arith.maxsi %[[UB]], %[[EDGE]] : index
+// CHECK: acc.loop control(%{{.*}} : index) = (%[[IV]] : index) to (%[[MAX_UB]] : index) step (%{{.*}} : index) {
+func.func @descending_loop_tile(%arg0: memref<16xf32>, %start: index, %floor: index) {
+ %cm1 = arith.constant -1 : index
+ %c4 = arith.constant 4 : index
+ acc.loop tile({%c4 : index}) control(%i : index) = (%start : index) to (%floor : index) step (%cm1 : index) {
+ %val = arith.index_castui %i : index to i32
+ %fval = arith.sitofp %val : i32 to f32
+ memref.store %fval, %arg0[%i] : memref<16xf32>
+ acc.yield
+ } independent
+ return
+}
+
+// Test a descending loop with an inclusive upper bound: the inclusive edge
+// moves in the step's direction, so it adds one instead of subtracting.
+
+// CHECK-LABEL: func.func @descending_inclusive_tile
+// CHECK: %[[CM32:.*]] = arith.constant -32 : index
+// CHECK: acc.loop control(%[[IV:.*]] : index) = (%{{.*}} : index) to (%[[UB:.*]] : index) step (%[[CM32]] : index) {
+// CHECK: %[[STEPPED:.*]] = arith.addi %[[IV]], %[[CM32]] : index
+// CHECK: %[[C1:.*]] = arith.constant 1 : index
+// CHECK: %[[EDGE:.*]] = arith.addi %[[STEPPED]], %[[C1]] : index
+// CHECK: %[[MAX_UB:.*]] = arith.maxsi %[[UB]], %[[EDGE]] : index
+// CHECK: acc.loop control(%{{.*}} : index) = (%[[IV]] : index) to (%[[MAX_UB]] : index) step (%{{.*}} : index) {
+func.func @descending_inclusive_tile(%arg0: memref<128xf32>, %start: index, %floor: index) {
+ %cm1 = arith.constant -1 : index
+ %c32 = arith.constant 32 : index
+ acc.loop tile({%c32 : index}) control(%i : index) = (%start : index) to (%floor : index) step (%cm1 : index) {
+ %val = arith.index_castui %i : index to i32
+ %fval = arith.sitofp %val : i32 to f32
+ memref.store %fval, %arg0[%i] : memref<128xf32>
+ acc.yield
+ } inclusiveUpperbound(array<i1: true>) independent
+ return
+}
+
+// Test an ascending loop with an inclusive upper bound: uses min, and the edge
+// subtracts one.
+
+// CHECK-LABEL: func.func @ascending_inclusive_tile
+// CHECK: acc.loop control(%[[IV:.*]] : index) = (%{{.*}} : index) to (%[[UB:.*]] : index) step (%{{.*}} : index) {
+// CHECK: %[[STEPPED:.*]] = arith.addi %[[IV]], %{{.*}} : index
+// CHECK: %[[C1:.*]] = arith.constant 1 : index
+// CHECK: %[[EDGE:.*]] = arith.subi %[[STEPPED]], %[[C1]] : index
+// CHECK: %[[MIN_UB:.*]] = arith.minsi %[[UB]], %[[EDGE]] : index
+func.func @ascending_inclusive_tile(%arg0: memref<128xf32>, %lb: index, %ub: index) {
+ %c1 = arith.constant 1 : index
+ %c32 = arith.constant 32 : index
+ acc.loop tile({%c32 : index}) control(%i : index) = (%lb : index) to (%ub : index) step (%c1 : index) {
+ %val = arith.index_castui %i : index to i32
+ %fval = arith.sitofp %val : i32 to f32
+ memref.store %fval, %arg0[%i] : memref<128xf32>
+ acc.yield
+ } inclusiveUpperbound(array<i1: true>) independent
+ return
+}
+
+// Test a step whose sign is only known at run time: both clamps are computed
+// and selected on the sign.
+
+// CHECK-LABEL: func.func @runtime_step_tile
+// CHECK: acc.loop control(%[[IV:.*]] : index) = (%{{.*}} : index) to (%[[UB:.*]] : index) step (%{{.*}} : index) {
+// CHECK: %[[EDGE:.*]] = arith.addi %[[IV]], %{{.*}} : index
+// CHECK: %[[ZERO:.*]] = arith.constant 0 : index
+// CHECK: %[[IS_DESC:.*]] = arith.cmpi slt, %[[STEP:.*]], %[[ZERO]] : index
+// CHECK: %[[MAX_UB:.*]] = arith.maxsi %[[UB]], %[[EDGE]] : index
+// CHECK: %[[MIN_UB:.*]] = arith.minsi %[[UB]], %[[EDGE]] : index
+// CHECK: %[[SEL:.*]] = arith.select %[[IS_DESC]], %[[MAX_UB]], %[[MIN_UB]] : index
+// CHECK: acc.loop control(%{{.*}} : index) = (%[[IV]] : index) to (%[[SEL]] : index) step (%{{.*}} : index) {
+func.func @runtime_step_tile(%arg0: memref<16xf32>, %lb: index, %ub: index, %step: index) {
+ %c4 = arith.constant 4 : index
+ acc.loop tile({%c4 : index}) control(%i : index) = (%lb : index) to (%ub : index) step (%step : index) {
+ %val = arith.index_castui %i : index to i32
+ %fval = arith.sitofp %val : i32 to f32
+ memref.store %fval, %arg0[%i] : memref<16xf32>
+ acc.yield
+ } independent
+ return
+}
More information about the Mlir-commits
mailing list