From 0fbf4d81dd338828be67010bde59bfd774c4f5ff Mon Sep 17 00:00:00 2001
From: Vinicius Couto Espindola <vini.couto.e@gmail.com>
Date: Wed, 6 Dec 2023 11:14:00 -0300
Subject: [PATCH] [CIR][IR] Refactor ScopeOp assembly format

This simplifies and modularizes the assembly format for ScopeOp by
using the Tablegen assembly description and a new custom printer/parser
that handles regions with omitted terminators.

It also fixes an issue where the parser would not correctly handle
`cir.scopes` with a return value.

ghstack-source-id: c5b9be705113c21117363cb3bd78e19d133c3fc5
Pull Request resolved: https://github.com/llvm/clangir/pull/311
---
 clang/include/clang/CIR/Dialect/IR/CIROps.td |  6 ++-
 clang/lib/CIR/Dialect/IR/CIRDialect.cpp      | 53 +++++++++-----------
 clang/test/CIR/IR/scope.cir                  | 27 ++++++++++
 3 files changed, 54 insertions(+), 32 deletions(-)
 create mode 100644 clang/test/CIR/IR/scope.cir

diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 6448b4b3f379..74f0cdbf7663 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -715,12 +715,14 @@ def ScopeOp : CIR_Op<"scope", [
     will be inserted implicitly.
   }];
 
-  let results = (outs Variadic<AnyType>:$results);
+  let results = (outs Optional<AnyType>:$results);
   let regions = (region AnyRegion:$scopeRegion);
 
-  let hasCustomAssemblyFormat = 1;
   let hasVerifier = 1;
   let skipDefaultBuilders = 1;
+  let assemblyFormat = [{
+    custom<OmittedTerminatorRegion>($scopeRegion) (`:` type($results)^)? attr-dict
+  }];
 
   let builders = [
     // Scopes for yielding values.
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 5ba3da343369..6a98820f925a 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -185,6 +185,28 @@ bool omitRegionTerm(mlir::Region &r) {
   return singleNonEmptyBlock && yieldsNothing();
 }
 
+//===----------------------------------------------------------------------===//
+// CIR Custom Parsers/Printers
+//===----------------------------------------------------------------------===//
+
+static mlir::ParseResult
+parseOmittedTerminatorRegion(mlir::OpAsmParser &parser, mlir::Region &region) {
+  auto regionLoc = parser.getCurrentLocation();
+  if (parser.parseRegion(region))
+    return failure();
+  if (ensureRegionTerm(parser, region, regionLoc).failed())
+    return failure();
+  return success();
+}
+
+static void printOmittedTerminatorRegion(mlir::OpAsmPrinter &printer,
+                                          mlir::cir::ScopeOp &op,
+                                          mlir::Region &region) {
+  printer.printRegion(region,
+                      /*printEntryBlockArgs=*/false,
+                      /*printBlockTerminators=*/!omitRegionTerm(region));
+}
+
 //===----------------------------------------------------------------------===//
 // AllocaOp
 //===----------------------------------------------------------------------===//
@@ -581,35 +603,6 @@ LogicalResult IfOp::verify() { return success(); }
 // ScopeOp
 //===----------------------------------------------------------------------===//
 
-ParseResult cir::ScopeOp::parse(OpAsmParser &parser, OperationState &result) {
-  // Create one region within 'scope'.
-  result.regions.reserve(1);
-  Region *scopeRegion = result.addRegion();
-  auto loc = parser.getCurrentLocation();
-
-  // Parse the scope region.
-  if (parser.parseRegion(*scopeRegion, /*arguments=*/{}, /*argTypes=*/{}))
-    return failure();
-
-  if (ensureRegionTerm(parser, *scopeRegion, loc).failed())
-    return failure();
-
-  // Parse the optional attribute list.
-  if (parser.parseOptionalAttrDict(result.attributes))
-    return failure();
-  return success();
-}
-
-void cir::ScopeOp::print(OpAsmPrinter &p) {
-  p << ' ';
-  auto &scopeRegion = this->getScopeRegion();
-  p.printRegion(scopeRegion,
-                /*printEntryBlockArgs=*/false,
-                /*printBlockTerminators=*/!omitRegionTerm(scopeRegion));
-
-  p.printOptionalAttrDict(getOperation()->getAttrs());
-}
-
 /// Given the region at `index`, or the parent operation if `index` is None,
 /// return the successor regions. These are the regions that may be selected
 /// during the flow of control. `operands` is a set of optional attributes that
@@ -619,7 +612,7 @@ void ScopeOp::getSuccessorRegions(mlir::RegionBranchPoint point,
                                   SmallVectorImpl<RegionSuccessor> &regions) {
   // The only region always branch back to the parent operation.
   if (!point.isParent()) {
-    regions.push_back(RegionSuccessor(getResults()));
+    regions.push_back(RegionSuccessor(getODSResults(0)));
     return;
   }
 
diff --git a/clang/test/CIR/IR/scope.cir b/clang/test/CIR/IR/scope.cir
new file mode 100644
index 000000000000..0cc45c8e389b
--- /dev/null
+++ b/clang/test/CIR/IR/scope.cir
@@ -0,0 +1,27 @@
+// RUN: cir-opt %s -o %t.cir
+// RUN: FileCheck --input-file=%t.cir %s
+!u32i = !cir.int<u, 32>
+
+module {
+  // Should properly print/parse scope with implicit empty yield.
+  cir.func @implicit_yield() {
+    cir.scope {
+    }
+    // CHECK: cir.scope {
+    // CHECK: }
+    cir.return
+  }
+
+  // Should properly print/parse scope with explicit yield.
+  cir.func @explicit_yield() {
+    %0 = cir.scope {
+      %1 = cir.alloca !u32i, cir.ptr <!u32i>, ["a", init] {alignment = 4 : i64}
+      cir.yield %1 : !cir.ptr<!u32i>
+    } : !cir.ptr<!u32i>
+    // CHECK: %0 = cir.scope {
+    //          [...]
+    // CHECK:   cir.yield %1 : !cir.ptr<!u32i>
+    // CHECK: } : !cir.ptr<!u32i>
+    cir.return
+  }
+}