[mlir][acc] Effectively serial check should permit missing dimension

[razvanlupusoru]

The isEffectivelySerial check for acc.compute_region required that all parallelism dimensions be set to 1. However, missing dimensions means that the level of parallelism is not assigned and thus should not be considered. Fix the issue and add unit tests for the API.

[llvmorg-github-actions]

@llvm/pr-subscribers-mlir-openacc
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-openacc

Author: Razvan Lupusoru (razvanlupusoru)

Changes

The isEffectivelySerial check for acc.compute_region required that all parallelism dimensions be set to 1. However, missing dimensions means that the level of parallelism is not assigned and thus should not be considered. Fix the issue and add unit tests for the API.

---

Full diff: https://github.com/llvm/llvm-project/pull/194981.diff

2 Files Affected:

• (modified) mlir/lib/Dialect/OpenACC/IR/OpenACCCG.cpp (+4)
• (modified) mlir/unittests/Dialect/OpenACC/OpenACCCGOpsTest.cpp (+67)

diff --git a/mlir/lib/Dialect/OpenACC/IR/OpenACCCG.cpp b/mlir/lib/Dialect/OpenACC/IR/OpenACCCG.cpp
index fe806186ce25d..4d06b1f1ff90e 100644
--- a/mlir/lib/Dialect/OpenACC/IR/OpenACCCG.cpp
+++ b/mlir/lib/Dialect/OpenACC/IR/OpenACCCG.cpp
@@ -506,6 +506,10 @@ bool ComputeRegionOp::isEffectivelySerial() {
     return true;
 
   auto checkDim = [&](GPUParallelDimAttr dim) -> bool {
+    // Launch dimensions without an explicit `acc.par_width` for that dimension
+    // means that no such parallelism is assigned and thus defaults to width 1.
+    if (!getLaunchArg(dim))
+      return true;
     auto val = getKnownConstantLaunchArg(dim);
     return val && *val == 1;
   };
diff --git a/mlir/unittests/Dialect/OpenACC/OpenACCCGOpsTest.cpp b/mlir/unittests/Dialect/OpenACC/OpenACCCGOpsTest.cpp
index 44283c9091b4d..800d3b9a3108d 100644
--- a/mlir/unittests/Dialect/OpenACC/OpenACCCGOpsTest.cpp
+++ b/mlir/unittests/Dialect/OpenACC/OpenACCCGOpsTest.cpp
@@ -93,11 +93,78 @@ class OpenACCCGOpsTest : public ::testing::Test {
     YieldOp::create(regionBuilder, loc);
   }
 
+  /// Build a single `acc.compute_region` with the given launch arguments.
+  ComputeRegionOp makeComputeRegion(HostContext &host, ValueRange launchArgs) {
+    Region sourceRegion;
+    populateSourceRegionSingleBlock(sourceRegion, context, loc, std::nullopt,
+                                    false);
+    IRMapping mapping;
+    // Setting an empty origin since it should not be relevant for the tests.
+    ComputeRegionOp cr = buildComputeRegion(
+        loc, launchArgs, {}, "", sourceRegion, host.rewriter, mapping);
+    return cr;
+  }
+
   MLIRContext context;
   OpBuilder b;
   Location loc;
 };
 
+//===----------------------------------------------------------------------===//
+// ComputeRegionOp::isEffectivelySerial
+//===----------------------------------------------------------------------===//
+
+TEST_F(OpenACCCGOpsTest, IsEffectivelySerialNoLaunchArgs) {
+  HostContext host(context, loc, b);
+  ComputeRegionOp cr = makeComputeRegion(host, {});
+  EXPECT_TRUE(cr.isEffectivelySerial());
+  EXPECT_TRUE(succeeded(host.module->verify()));
+}
+
+TEST_F(OpenACCCGOpsTest, IsEffectivelySerialSparseDimsConstantOne) {
+  HostContext host(context, loc, b);
+  Value c1 = arith::ConstantIndexOp::create(host.rewriter, loc, 1);
+  Value pwBx = ParWidthOp::create(host.rewriter, loc, c1,
+                                  GPUParallelDimAttr::blockXDim(&context));
+  Value pwTy = ParWidthOp::create(host.rewriter, loc, c1,
+                                  GPUParallelDimAttr::threadYDim(&context));
+  Value pwTx = ParWidthOp::create(host.rewriter, loc, c1,
+                                  GPUParallelDimAttr::threadXDim(&context));
+  ComputeRegionOp cr = makeComputeRegion(host, {pwBx, pwTy, pwTx});
+  EXPECT_TRUE(cr.isEffectivelySerial());
+  EXPECT_TRUE(succeeded(host.module->verify()));
+}
+
+TEST_F(OpenACCCGOpsTest,
+       IsEffectivelySerialFalseWhenThreadXWidthGreaterThanOne) {
+  HostContext host(context, loc, b);
+  Value c2 = arith::ConstantIndexOp::create(host.rewriter, loc, 2);
+  Value pwTx = ParWidthOp::create(host.rewriter, loc, c2,
+                                  GPUParallelDimAttr::threadXDim(&context));
+  ComputeRegionOp cr = makeComputeRegion(host, pwTx);
+  EXPECT_FALSE(cr.isEffectivelySerial());
+  EXPECT_TRUE(succeeded(host.module->verify()));
+}
+
+TEST_F(OpenACCCGOpsTest, IsEffectivelySerialFalseWhenLaunchWidthUnknown) {
+  HostContext host(context, loc, b);
+  Value pwTx = ParWidthOp::create(host.rewriter, loc, Value(),
+                                  GPUParallelDimAttr::threadXDim(&context));
+  ComputeRegionOp cr = makeComputeRegion(host, pwTx);
+  EXPECT_FALSE(cr.isEffectivelySerial());
+  EXPECT_TRUE(succeeded(host.module->verify()));
+}
+
+TEST_F(OpenACCCGOpsTest, IsEffectivelySerialTrueWhenSeqLaunchPresent) {
+  HostContext host(context, loc, b);
+  Value c7 = arith::ConstantIndexOp::create(host.rewriter, loc, 7);
+  Value pwSeq = ParWidthOp::create(host.rewriter, loc, c7,
+                                   GPUParallelDimAttr::seqDim(&context));
+  ComputeRegionOp cr = makeComputeRegion(host, pwSeq);
+  EXPECT_TRUE(cr.isEffectivelySerial());
+  EXPECT_TRUE(succeeded(host.module->verify()));
+}
+
 //===----------------------------------------------------------------------===//
 // ComputeRegionOp::wireHoistedValueThroughIns
 //===----------------------------------------------------------------------===//

[razvanlupusoru]

I am abandoning this in favor of #195158