[CIR] Properly ensure terminating IfOp and ScopeOp regions

The code changes modify the `cir.if` and `cir.scope` operations to ensure that their code regions are properly terminated. Previously, the if/else and scope regions could be left completely empty which is non-trivially expected in most code inspecting these ops. This led, for example, to a crash when and if clause was left empty in the source code.

Now, the child regions must be terminated, either explicitly or implicitly by the default builder and assembly parser. This change improves the clarity and correctness of the code.

Author: orbiri

clang/include/clang/CIR/Dialect/IR/CIROps.td

@@ -750,8 +750,10 @@ def IfOp : CIR_Op<"if",
     }
     ```
 
-    `cir.if` defines no values and the 'else' can be omitted. `cir.yield` must
-    explicitly terminate the region if it has more than one block.
+    `cir.if` defines no values and the 'else' can be omitted. The if/else
+    regions must be terminated. If the region has only one block, the terminator
+    can be left out, and `cir.yield` terminator will be inserted implictly.
+    Otherwise, the region must be explicitly terminated.
   }];
   let arguments = (ins CIR_BoolType:$condition);
   let regions = (region AnyRegion:$thenRegion, AnyRegion:$elseRegion);
@@ -1070,6 +1072,16 @@ def ScopeOp : CIR_Op<"scope", [
     custom<OmittedTerminatorRegion>($scopeRegion) (`:` type($results)^)? attr-dict
   }];
 
+  let extraClassDeclaration = [{
+    /// Determine whether the scope is empty, meaning it contains a single block
+    /// terminated by a cir.yield.
+    bool isEmpty() {
+      auto &entry = getRegion().front();
+      return getRegion().hasOneBlock() &&
+        llvm::isa<YieldOp>(entry.front());
+      }
+    }];
+
   let builders = [
     // Scopes for yielding values.
     OpBuilder<(ins
@@ -1200,7 +1212,7 @@ def ShiftOp : CIR_Op<"shift", [Pure]> {
     be either integer type or vector of integer type. However, they must be
     either all vector of integer type, or all integer type. If they are vectors,
     each vector element of the shift target is shifted by the corresponding
-    shift amount in the shift amount vector. 
+    shift amount in the shift amount vector.
 
     ```mlir
     %7 = cir.shift(left, %1 : !u64i, %4 : !s32i) -> !u64i
@@ -1879,17 +1891,17 @@ def SwitchOp : CIR_Op<"switch",
     is an integral condition value.
 
     The set of `cir.case` operations and their enclosing `cir.switch`
-    represents the semantics of a C/C++ switch statement. Users can use 
+    represents the semantics of a C/C++ switch statement. Users can use
     `collectCases(llvm::SmallVector<CaseOp> &cases)` to collect the `cir.case`
     operation in the `cir.switch` operation easily.
 
     The `cir.case` operations doesn't have to be in the region of `cir.switch`
     directly. However, when all the `cir.case` operations lives in the region
     of `cir.switch` directly and there is no other operations except the ending
-    `cir.yield` operation in the region of `cir.switch` directly, we call the 
-    `cir.switch` operation is in a simple form. Users can use 
+    `cir.yield` operation in the region of `cir.switch` directly, we call the
+    `cir.switch` operation is in a simple form. Users can use
     `bool isSimpleForm(llvm::SmallVector<CaseOp> &cases)` member function to
-    detect if the `cir.switch` operation is in a simple form. The simple form 
+    detect if the `cir.switch` operation is in a simple form. The simple form
     makes analysis easier to handle the `cir.switch` operation
     and makes the boundary to give up pretty clear.
 
@@ -1976,7 +1988,7 @@ def SwitchOp : CIR_Op<"switch",
     switch(int cond) {
       l:
         b++;
-  
+
       case 4:
         a++;
         break;
@@ -4136,13 +4148,13 @@ def ReturnAddrOp : CIR_Op<"return_address"> {
   let results = (outs Res<VoidPtr, "">:$result);
 
   let description = [{
-    Represents call to builtin function ` __builtin_return_address` in CIR. 
-    This builtin function returns the return address of the current function, 
-    or of one of its callers. 
+    Represents call to builtin function ` __builtin_return_address` in CIR.
+    This builtin function returns the return address of the current function,
+    or of one of its callers.
     The `level` argument is number of frames to scan up the call stack.
-    For instance, value of 0 yields the return address of the current function, 
-    value of 1 yields the return address of the caller of the current function, 
-    and so forth. 
+    For instance, value of 0 yields the return address of the current function,
+    value of 1 yields the return address of the caller of the current function,
+    and so forth.
 
     Examples:
 
@@ -4282,8 +4294,8 @@ def AbsOp : CIR_Op<"abs", [Pure, SameOperandsAndResultType]> {
   let summary = [{
     libc builtin equivalent abs, labs, llabs
 
-    The `poison` argument indicate whether the result value 
-    is a poison value if the first argument is statically or 
+    The `poison` argument indicate whether the result value
+    is a poison value if the first argument is statically or
     dynamically an INT_MIN value.
 
     Example:

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -208,20 +208,24 @@ LogicalResult ensureRegionTerm(OpAsmParser &parser, Region &region,
   Location eLoc = parser.getEncodedSourceLoc(parser.getCurrentLocation());
   OpBuilder builder(parser.getBuilder().getContext());
 
-  // Region is empty or properly terminated: nothing to do.
-  if (region.empty() ||
-      (region.back().mightHaveTerminator() && region.back().getTerminator()))
+  // Insert empty block in case the region is empty to ensure the terminator
+  // will be inserted
+  if (region.empty())
+    builder.createBlock(&region);
+
+  Block &block = region.back();
+  // Region is properly terminated: nothing to do.
+  if (!block.empty() && block.back().hasTrait<OpTrait::IsTerminator>())
     return success();
 
   // Check for invalid terminator omissions.
   if (!region.hasOneBlock())
     return parser.emitError(errLoc,
                             "multi-block region must not omit terminator");
-  if (region.back().empty())
-    return parser.emitError(errLoc, "empty region must not omit terminator");
 
-  // Terminator was omited correctly: recreate it.
-  region.back().push_back(builder.create<cir::YieldOp>(eLoc));
+  // Terminator was omitted correctly: recreate it.
+  builder.setInsertionPointToEnd(&block);
+  builder.create<cir::YieldOp>(eLoc);
   return success();
 }
 
@@ -1126,8 +1130,11 @@ void cir::IfOp::print(OpAsmPrinter &p) {
   p.printOptionalAttrDict(getOperation()->getAttrs());
 }
 
-/// Default callback for IfOp builders. Inserts nothing for now.
-void cir::buildTerminatedBody(OpBuilder &builder, Location loc) {}
+/// Default callback for IfOp builders.
+void cir::buildTerminatedBody(OpBuilder &builder, Location loc) {
+  // add cir.yield to the end of the block
+  builder.create<cir::YieldOp>(loc);
+}
 
 /// 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
@@ -1236,7 +1243,17 @@ void cir::ScopeOp::build(
   scopeBuilder(builder, result.location);
 }
 
-LogicalResult cir::ScopeOp::verify() { return success(); }
+LogicalResult cir::ScopeOp::verify() {
+  if (getRegion().empty()) {
+    return emitOpError() << "cir.scope must not be empty since it should "
+                            "include at least an implicit cir.yield ";
+  }
+
+  if (getRegion().back().empty() ||
+      !getRegion().back().getTerminator()->hasTrait<OpTrait::IsTerminator>())
+    return emitOpError() << "last block of cir.scope must be terminated";
+  return success();
+}
 
 //===----------------------------------------------------------------------===//
 // TryOp

clang/lib/CIR/Dialect/Transforms/CIRCanonicalize.cpp

@@ -60,9 +60,9 @@ struct RemoveEmptyScope : public OpRewritePattern<ScopeOp> {
   using OpRewritePattern<ScopeOp>::OpRewritePattern;
 
   LogicalResult match(ScopeOp op) const final {
-    return success(op.getRegion().empty() ||
-                   (op.getRegion().getBlocks().size() == 1 &&
-                    op.getRegion().front().empty()));
+    // TODO: Remove this logic once CIR uses MLIR infrastructure to remove
+    // trivially dead operations
+    return success(op.isEmpty());
   }
 
   void rewrite(ScopeOp op, PatternRewriter &rewriter) const final {

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

@@ -66,7 +66,7 @@ struct CIRIfFlattening : public OpRewritePattern<IfOp> {
     auto *remainingOpsBlock =
         rewriter.splitBlock(currentBlock, rewriter.getInsertionPoint());
     mlir::Block *continueBlock;
-    if (ifOp->getResults().size() == 0)
+    if (ifOp->getResults().empty())
       continueBlock = remainingOpsBlock;
     else
       llvm_unreachable("NYI");
@@ -125,7 +125,9 @@ class CIRScopeOpFlattening : public mlir::OpRewritePattern<cir::ScopeOp> {
     auto loc = scopeOp.getLoc();
 
     // Empty scope: just remove it.
-    if (scopeOp.getRegion().empty()) {
+    // TODO: Remove this logic once CIR uses MLIR infrastructure to remove
+    // trivially dead operations
+    if (scopeOp.isEmpty()) {
       rewriter.eraseOp(scopeOp);
       return mlir::success();
     }

clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp

@@ -787,7 +787,9 @@ class CIRScopeOpLowering : public mlir::OpConversionPattern<cir::ScopeOp> {
   matchAndRewrite(cir::ScopeOp scopeOp, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     // Empty scope: just remove it.
-    if (scopeOp.getRegion().empty()) {
+    // TODO: Remove this logic once CIR uses MLIR infrastructure to remove
+    // trivially dead operations
+    if (scopeOp.isEmpty()) {
       rewriter.eraseOp(scopeOp);
       return mlir::success();
     }

clang/test/CIR/CodeGen/OpenMP/parallel.cpp

@@ -4,8 +4,6 @@
 // CHECK: cir.func
 void omp_parallel_1() {
 // CHECK: omp.parallel {
-// CHECK-NEXT: cir.scope {
-// CHECK-NEXT: }
 // CHECK-NEXT: omp.terminator
 // CHECK-NEXT: }
 #pragma omp parallel

clang/test/CIR/CodeGen/if-constexpr.cpp

@@ -75,9 +75,6 @@ void if0() {
 // CHECK-NEXT:   cir.store %3, %2 : !s32i, !cir.ptr<!s32i> loc({{.*}})
 // CHECK-NEXT: } loc({{.*}})
 // CHECK-NEXT: cir.scope {
-// Note that Clang does not even emit a block in this case
-// CHECK-NEXT: } loc({{.*}})
-// CHECK-NEXT: cir.scope {
 // CHECK-NEXT:   %2 = cir.alloca !s32i, !cir.ptr<!s32i>, ["x", init] {{.*}}
 // CHECK-NEXT:   %3 = cir.alloca !s32i, !cir.ptr<!s32i>, ["y", init] {{.*}}
 // CHECK-NEXT:   %4 = cir.const #cir.int<70> : !s32i loc({{.*}})

clang/test/CIR/CodeGen/stmt-expr.c

@@ -4,9 +4,8 @@
 // Yields void.
 void test1() { ({ }); }
 // CHECK: @test1
-//     CHECK: cir.scope {
-// CHECK-NOT:   cir.yield
-//     CHECK: }
+//     CHECK-NEXT: cir.return
+
 
 // Yields an out-of-scope scalar.
 void test2() { ({int x = 3; x; }); }

clang/test/CIR/Lowering/if.cir

@@ -62,4 +62,38 @@ module {
   // MLIR-NEXT: ^bb2:  // pred: ^bb0
   // MLIR-NEXT:   llvm.return %arg0 : i32
   // MLIR-NEXT: }
+
+  // Verify empty if clause is properly lowered to empty block
+  cir.func @emptyIfClause(%arg0: !s32i) -> !s32i {
+    // MLIR-LABEL: llvm.func @emptyIfClause
+    %4 = cir.cast(int_to_bool, %arg0 : !s32i), !cir.bool
+    // MLIR: llvm.cond_br {{%.*}}, ^[[T:.*]], ^[[PHI:.*]]
+    cir.if %4 {
+      // MLIR-NEXT: ^[[T]]:
+      // MLIR-NEXT:   llvm.br ^[[PHI]]
+    }
+    // MLIR-NEXT: ^[[PHI]]:
+    // MLIR-NEXT:   llvm.return
+    cir.return %arg0 : !s32i
+  }
+
+  // Verify empty if-else clauses are properly lowered to empty blocks
+  // TODO: Fix reversed order of blocks in the test once Issue clangir/#1094 is
+  // addressed
+  cir.func @emptyIfElseClause(%arg0: !s32i) -> !s32i {
+    // MLIR-LABEL: llvm.func @emptyIfElseClause
+    %4 = cir.cast(int_to_bool, %arg0 : !s32i), !cir.bool
+    // MLIR: llvm.cond_br {{%.*}}, ^[[T:.*]], ^[[F:.*]]
+    cir.if %4 {
+    } else {
+    }
+    // MLIR-NEXT: ^[[F]]:
+    // MLIR-NEXT:   llvm.br ^[[PHI:.*]]
+    // MLIR-NEXT: ^[[T]]:
+    // MLIR-NEXT:   llvm.br ^[[PHI]]
+    // MLIR-NEXT: ^[[PHI]]:
+    // MLIR-NEXT:   llvm.return
+    cir.return %arg0 : !s32i
+  }
+
 }