[CIR] Upstream initial support for switch statements #137106

Andres-Salamanca · 2025-04-24T01:28:49Z

This introduces initial support for the switchOP, with caseOP of the Equal kind. Support for additional case kinds such as AnyOf, Range, and Default will be included in a future patch. Similarly, lowering to LLVM IR and flattening of the switch are deferred and will be addressed in subsequent updates.

llvmbot · 2025-04-24T01:29:22Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: None (Andres-Salamanca)

Changes

This introduces initial support for the switchOP, with caseOP of the Equal kind. Support for additional case kinds such as AnyOf, Range, and Default will be included in a future patch. Similarly, lowering to LLVM IR and flattening of the switch are deferred and will be addressed in subsequent updates.

Patch is 28.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/137106.diff

7 Files Affected:

(modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+224-3)
(modified) clang/include/clang/CIR/MissingFeatures.h (+1)
(modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+18)
(modified) clang/lib/CIR/CodeGen/CIRGenStmt.cpp (+213-1)
(modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+129-1)
(added) clang/test/CIR/CodeGen/switch.cpp (+92)
(added) clang/test/CIR/IR/switch.cir (+38)

diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index bb19de31b4fa5..25cdf156659c7 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -470,7 +470,8 @@ def StoreOp : CIR_Op<"store", [
 //===----------------------------------------------------------------------===//
 
 def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "IfOp",
-                                              "DoWhileOp", "WhileOp", "ForOp"]>,
+                                              "SwitchOp", "DoWhileOp","WhileOp",
+                                              "ForOp", "CaseOp"]>,
                                  Terminator]> {
   let summary = "Return from function";
   let description = [{
@@ -609,8 +610,9 @@ def ConditionOp : CIR_Op<"condition", [
 //===----------------------------------------------------------------------===//
 
 def YieldOp : CIR_Op<"yield", [ReturnLike, Terminator,
-                               ParentOneOf<["IfOp", "ScopeOp", "WhileOp",
-                                            "ForOp", "DoWhileOp"]>]> {
+                               ParentOneOf<["IfOp", "ScopeOp", "SwitchOp",
+                                            "WhileOp", "ForOp", "CaseOp",
+                                            "DoWhileOp"]>]> {
   let summary = "Represents the default branching behaviour of a region";
   let description = [{
     The `cir.yield` operation terminates regions on different CIR operations,
@@ -753,6 +755,225 @@ def ScopeOp : CIR_Op<"scope", [
   ];
 }
 
+//===----------------------------------------------------------------------===//
+// SwitchOp
+//===----------------------------------------------------------------------===//
+
+def CaseOpKind_DT : I32EnumAttrCase<"Default", 1, "default">;
+def CaseOpKind_EQ : I32EnumAttrCase<"Equal", 2, "equal">;
+def CaseOpKind_AO : I32EnumAttrCase<"Anyof", 3, "anyof">;
+def CaseOpKind_RG : I32EnumAttrCase<"Range", 4, "range">;
+
+def CaseOpKind : I32EnumAttr<
+    "CaseOpKind",
+    "case kind",
+    [CaseOpKind_DT, CaseOpKind_EQ, CaseOpKind_AO, CaseOpKind_RG]> {
+  let cppNamespace = "::cir";
+}
+
+def CaseOp : CIR_Op<"case", [
+       DeclareOpInterfaceMethods<RegionBranchOpInterface>,
+       RecursivelySpeculatable, AutomaticAllocationScope]> {
+  let summary = "Case operation";
+  let description = [{
+    The `cir.case` operation represents a case within a C/C++ switch.
+    The `cir.case` operation must be in a `cir.switch` operation directly
+    or indirectly.
+
+    The `cir.case` have 4 kinds:
+    - `equal, <constant>`: equality of the second case operand against the
+    condition.
+    - `anyof, [constant-list]`: equals to any of the values in a subsequent
+    following list.
+    - `range, [lower-bound, upper-bound]`: the condition is within the closed
+                                           interval.
+    - `default`: any other value.
+
+    Each case region must be explicitly terminated.
+  }];
+
+  let arguments = (ins ArrayAttr:$value, CaseOpKind:$kind);
+  let regions = (region AnyRegion:$caseRegion);
+
+  let assemblyFormat = "`(` $kind `,` $value `)` $caseRegion attr-dict";
+
+  let hasVerifier = 1;
+
+  let skipDefaultBuilders = 1;
+let builders = [
+    OpBuilder<(ins "mlir::ArrayAttr":$value,
+                   "CaseOpKind":$kind,
+                   "mlir::OpBuilder::InsertPoint &":$insertPoint)>
+  ];
+}
+
+def SwitchOp : CIR_Op<"switch",
+      [SameVariadicOperandSize,
+       DeclareOpInterfaceMethods<RegionBranchOpInterface>,
+       RecursivelySpeculatable, AutomaticAllocationScope, NoRegionArguments]> {
+  let summary = "Switch operation";
+  let description = [{
+    The `cir.switch` operation represents C/C++ switch functionality for
+    conditionally executing multiple regions of code. The operand to an 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
+    `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
+    `bool isSimpleForm(llvm::SmallVector<CaseOp> &cases)` member function to
+    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.
+
+    To make the simple form as common as possible, CIR code generation attaches
+    operations corresponding to the statements that lives between top level
+    cases into the closest `cir.case` operation.
+
+    For example,
+
+    ```
+    switch(int cond) {
+      case 4:
+        a++;
+
+      b++;
+      case 5;
+        c++;
+
+      ...
+    }
+    ```
+
+    The statement `b++` is not a sub-statement of the case statement `case 4`.
+    But to make the generated `cir.switch` a simple form, we will attach the
+    statement `b++` into the closest `cir.case` operation. So that the generated
+    code will be like:
+
+    ```
+    cir.switch(int cond) {
+      cir.case(equal, 4) {
+        a++;
+        b++;
+        cir.yield
+      }
+      cir.case(equal, 5) {
+        c++;
+        cir.yield
+      }
+      ...
+    }
+    ```
+
+    For the same reason, we will hoist the case statement as the substatement
+    of another case statement so that they will be in the same level. For
+    example,
+
+    ```
+    switch(int cond) {
+      case 4:
+      default;
+      case 5;
+        a++;
+      ...
+    }
+    ```
+
+    will be generated as
+
+    ```
+    cir.switch(int cond) {
+      cir.case(equal, 4) {
+        cir.yield
+      }
+      cir.case(default) {
+        cir.yield
+      }
+      cir.case(equal, 5) {
+        a++;
+        cir.yield
+      }
+      ...
+    }
+    ```
+
+    The cir.switch might not be considered "simple" if any of the following is
+    true:
+    - There are case statements of the switch statement lives in other scopes
+      other than the top level compound statement scope. Note that a case
+      statement itself doesn't form a scope.
+    - The sub-statement of the switch statement is not a compound statement.
+    - There are codes before the first case statement. For example,
+
+    ```
+    switch(int cond) {
+      l:
+        b++;
+
+      case 4:
+        a++;
+        break;
+
+      case 5:
+        goto l;
+      ...
+    }
+    ```
+
+    the generated CIR for this non-simple switch would be:
+
+    ```
+    cir.switch(int cond) {
+      cir.label "l"
+      b++;
+      cir.case(4) {
+        a++;
+        cir.break
+      }
+      cir.case(5) {
+        goto "l"
+      }
+      cir.yield
+    }
+    ```
+  }];
+
+  let arguments = (ins CIR_IntType:$condition);
+
+  let regions = (region AnyRegion:$body);
+
+  let hasVerifier = 1;
+
+  let skipDefaultBuilders = 1;
+  let builders = [
+    OpBuilder<(ins "mlir::Value":$condition,
+               "llvm::function_ref<void(mlir::OpBuilder &, mlir::Location, mlir::OperationState &)>":$switchBuilder)>
+  ];
+
+  let assemblyFormat = [{
+    custom<SwitchOp>(
+      $body, $condition, type($condition)
+    )
+    attr-dict
+  }];
+
+  let extraClassDeclaration = [{
+    // Collect cases in the switch.
+    void collectCases(llvm::SmallVector<CaseOp> &cases);
+
+    // Check if the switch is in a simple form.
+    // If yes, collect the cases to \param cases.
+    // This is an expensive and need to be used with caution.
+    bool isSimpleForm(llvm::SmallVector<CaseOp> &cases);
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // BrOp
 //===----------------------------------------------------------------------===//
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 6bfc1199aea55..4783468036ec3 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -159,6 +159,7 @@ struct MissingFeatures {
   static bool bitfields() { return false; }
   static bool typeChecks() { return false; }
   static bool lambdaFieldToName() { return false; }
+  static bool foldCaseStmt() { return false; }
 
   // Missing types
   static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index f533d0ab53cd2..f08a0521d7881 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -63,6 +63,9 @@ class CIRGenFunction : public CIRGenTypeCache {
   /// declarations.
   DeclMapTy localDeclMap;
 
+  /// The type of the condition for the emitting switch statement.
+  llvm::SmallVector<mlir::Type, 2> condTypeStack;
+
   clang::ASTContext &getContext() const { return cgm.getASTContext(); }
 
   CIRGenBuilderTy &getBuilder() { return builder; }
@@ -469,6 +472,16 @@ class CIRGenFunction : public CIRGenTypeCache {
                       ReturnValueSlot returnValue = ReturnValueSlot());
   CIRGenCallee emitCallee(const clang::Expr *e);
 
+  template <typename T>
+  mlir::LogicalResult emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
+                                             mlir::ArrayAttr value,
+                                             cir::CaseOpKind kind,
+                                             bool buildingTopLevelCase);
+
+  mlir::LogicalResult emitCaseStmt(const clang::CaseStmt &s,
+                                   mlir::Type condType,
+                                   bool buildingTopLevelCase);
+
   mlir::LogicalResult emitContinueStmt(const clang::ContinueStmt &s);
   mlir::LogicalResult emitDoStmt(const clang::DoStmt &s);
 
@@ -595,6 +608,11 @@ class CIRGenFunction : public CIRGenTypeCache {
 
   mlir::Value emitStoreThroughBitfieldLValue(RValue src, LValue dstresult);
 
+  mlir::LogicalResult emitSwitchBody(const clang::Stmt *s);
+  mlir::LogicalResult emitSwitchCase(const clang::SwitchCase &s,
+                                     bool buildingTopLevelCase);
+  mlir::LogicalResult emitSwitchStmt(const clang::SwitchStmt &s);
+
   /// Given a value and its clang type, returns the value casted to its memory
   /// representation.
   /// Note: CIR defers most of the special casting to the final lowering passes
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index 82ac53706b7f9..9c17821415f32 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -89,6 +89,8 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
     }
   case Stmt::IfStmtClass:
     return emitIfStmt(cast<IfStmt>(*s));
+  case Stmt::SwitchStmtClass:
+    return emitSwitchStmt(cast<SwitchStmt>(*s));
   case Stmt::ForStmtClass:
     return emitForStmt(cast<ForStmt>(*s));
   case Stmt::WhileStmtClass:
@@ -132,7 +134,6 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
   case Stmt::CaseStmtClass:
   case Stmt::SEHLeaveStmtClass:
   case Stmt::SYCLKernelCallStmtClass:
-  case Stmt::SwitchStmtClass:
   case Stmt::CoroutineBodyStmtClass:
   case Stmt::CoreturnStmtClass:
   case Stmt::CXXTryStmtClass:
@@ -422,6 +423,121 @@ mlir::LogicalResult CIRGenFunction::emitBreakStmt(const clang::BreakStmt &s) {
   return mlir::success();
 }
 
+template <typename T>
+mlir::LogicalResult
+CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
+                                       mlir::ArrayAttr value, CaseOpKind kind,
+                                       bool buildingTopLevelCase) {
+
+  assert((isa<CaseStmt, DefaultStmt>(stmt)) &&
+         "only case or default stmt go here");
+
+  mlir::LogicalResult result = mlir::success();
+
+  mlir::Location loc = getLoc(stmt->getBeginLoc());
+
+  enum class SubStmtKind { Case, Default, Other };
+  SubStmtKind subStmtKind = SubStmtKind::Other;
+  const Stmt *sub = stmt->getSubStmt();
+
+  mlir::OpBuilder::InsertPoint insertPoint;
+  builder.create<CaseOp>(loc, value, kind, insertPoint);
+
+  {
+    mlir::OpBuilder::InsertionGuard guardSwitch(builder);
+    builder.restoreInsertionPoint(insertPoint);
+
+    if (isa<DefaultStmt>(sub) && isa<CaseStmt>(stmt)) {
+      subStmtKind = SubStmtKind::Default;
+      builder.createYield(loc);
+    } else if (isa<CaseStmt>(sub) && isa<DefaultStmt>(stmt)) {
+      subStmtKind = SubStmtKind::Case;
+      builder.createYield(loc);
+    } else
+      result = emitStmt(sub, /*useCurrentScope=*/!isa<CompoundStmt>(sub));
+
+    insertPoint = builder.saveInsertionPoint();
+  }
+
+  // If the substmt is default stmt or case stmt, try to handle the special case
+  // to make it into the simple form. e.g.
+  //
+  //  swtich () {
+  //    case 1:
+  //    default:
+  //      ...
+  //  }
+  //
+  // we prefer generating
+  //
+  //  cir.switch() {
+  //     cir.case(equal, 1) {
+  //        cir.yield
+  //     }
+  //     cir.case(default) {
+  //        ...
+  //     }
+  //  }
+  //
+  // than
+  //
+  //  cir.switch() {
+  //     cir.case(equal, 1) {
+  //       cir.case(default) {
+  //         ...
+  //       }
+  //     }
+  //  }
+  //
+  // We don't need to revert this if we find the current switch can't be in
+  // simple form later since the conversion itself should be harmless.
+  if (subStmtKind == SubStmtKind::Case)
+    result = emitCaseStmt(*cast<CaseStmt>(sub), condType, buildingTopLevelCase);
+  else if (subStmtKind == SubStmtKind::Default) {
+    getCIRGenModule().errorNYI(sub->getSourceRange(), "Default case");
+    return mlir::failure();
+  } else if (buildingTopLevelCase)
+    // If we're building a top level case, try to restore the insert point to
+    // the case we're building, then we can attach more random stmts to the
+    // case to make generating `cir.switch` operation to be a simple form.
+    builder.restoreInsertionPoint(insertPoint);
+
+  return result;
+}
+
+mlir::LogicalResult CIRGenFunction::emitCaseStmt(const CaseStmt &s,
+                                                 mlir::Type condType,
+                                                 bool buildingTopLevelCase) {
+  llvm::APSInt intVal = s.getLHS()->EvaluateKnownConstInt(getContext());
+  SmallVector<mlir::Attribute, 1> caseEltValueListAttr;
+  caseEltValueListAttr.push_back(cir::IntAttr::get(condType, intVal));
+  mlir::ArrayAttr value = builder.getArrayAttr(caseEltValueListAttr);
+  if (s.getRHS()) {
+    getCIRGenModule().errorNYI(s.getSourceRange(), "SwitchOp range kind");
+    return mlir::failure();
+  }
+  assert(!cir::MissingFeatures::foldCaseStmt());
+  return emitCaseDefaultCascade(&s, condType, value, cir::CaseOpKind::Equal,
+                                buildingTopLevelCase);
+}
+
+mlir::LogicalResult CIRGenFunction::emitSwitchCase(const SwitchCase &s,
+                                                   bool buildingTopLevelCase) {
+  assert(!condTypeStack.empty() &&
+         "build switch case without specifying the type of the condition");
+
+  if (s.getStmtClass() == Stmt::CaseStmtClass)
+    return emitCaseStmt(cast<CaseStmt>(s), condTypeStack.back(),
+                        buildingTopLevelCase);
+
+  if (s.getStmtClass() == Stmt::DefaultStmtClass) {
+    getCIRGenModule().errorNYI(s.getSourceRange(), "Default case");
+    return mlir::failure();
+  }
+
+  llvm_unreachable("expect case or default stmt");
+}
+
 mlir::LogicalResult CIRGenFunction::emitForStmt(const ForStmt &s) {
   cir::ForOp forOp;
 
@@ -600,3 +716,99 @@ mlir::LogicalResult CIRGenFunction::emitWhileStmt(const WhileStmt &s) {
   terminateBody(builder, whileOp.getBody(), getLoc(s.getEndLoc()));
   return mlir::success();
 }
+
+mlir::LogicalResult CIRGenFunction::emitSwitchBody(const Stmt *s) {
+  // It is rare but legal if the switch body is not a compound stmt. e.g.,
+  //
+  //  switch(a)
+  //    while(...) {
+  //      case1
+  //      ...
+  //      case2
+  //      ...
+  //    }
+  if (!isa<CompoundStmt>(s))
+    return emitStmt(s, /*useCurrentScope=*/true);
+
+  auto *compoundStmt = cast<CompoundStmt>(s);
+
+  mlir::Block *swtichBlock = builder.getBlock();
+  for (auto *c : compoundStmt->body()) {
+    if (auto *switchCase = dyn_cast<SwitchCase>(c)) {
+      builder.setInsertionPointToEnd(swtichBlock);
+      // Reset insert point automatically, so that we can attach following
+      // random stmt to the region of previous built case op to try to make
+      // the being generated `cir.switch` to be in simple form.
+      if (mlir::failed(
+              emitSwitchCase(*switchCase, /*buildingTopLevelCase=*/true)))
+        return mlir::failure();
+
+      continue;
+    }
+
+    // Otherwise, just build the statements in the nearest case region.
+    if (mlir::failed(emitStmt(c, /*useCurrentScope=*/!isa<CompoundStmt>(c))))
+      return mlir::failure();
+  }
+
+  return mlir::success();
+}
+
+mlir::LogicalResult CIRGenFunction::emitSwitchStmt(const clang::SwitchStmt &s) {
+  // TODO: LLVM codegen does some early optimization to fold the condition and
+  // only emit live cases. CIR should use MLIR to achieve similar things,
+  // nothing to be done here.
+  // if (ConstantFoldsToSimpleInteger(S.getCond(), ConstantCondValue))...
+
+  SwitchOp swop;
+  auto switchStmtBuilder = [&]() -> mlir::LogicalResult {
+    if (s.getInit())
+      if (emitStmt(s.getInit(), /*useCurrentScope=*/true).failed())
+        return mlir::failure();
+
+    if (s.getConditionVariable())
+      emitDecl(*s.getConditionVariable());
+
+    mlir::Value condV = emitScalarExpr(s.getCond());
+
+    // TODO: PGO and likelihood (e.g. PGO.haveRegionCounts())
+    assert(!cir::MissingFeatures::pgoUse());
+    assert(!cir::MissingFeatures::emitCondLikelihoodViaExpectIntrinsic());
+    // TODO: if the switch has a condition wrapped by __builtin_unpredictable?
+    assert(!cir::MissingFeatures::insertBuiltinUnpredictable());
+
+    mlir::LogicalResult res = mlir::success();
+    swop = builder.create<SwitchOp>(
+        getLoc(s.getBeginLoc()), condV,
+        /*switchBuilder=*/
+        [&](mlir::OpBuilder &b, mlir::Location loc, mlir::OperationState &os) {
+          curLexScope->setAsSwitch();
+
+          condTypeStack.push_back(condV.getType());
+
+          res = emitSwitchBody(s.getBody());
+
+          condTypeStack.pop_back();
+        });
+
+    return res;
+  };
+
+  // The switch scope contains the full source range for SwitchStmt.
+  mlir::Location scopeLoc = getLoc(s.getSourceRange());
+  mlir::LogicalResult res = mlir::success();
+  builder.create<cir::ScopeOp>(scopeLoc, /*scopeBuilder=*/
+                               [&](mlir::OpBuilder &b, mlir::Location loc) {
+                                 LexicalScope lexScope{
+                                     *this, loc, builder.getInsertionBlock()};
+                                 res = switchStmtBuilder();
+                               });
+
+  llvm::SmallVector<CaseOp> cases;
+  swop.collectCases(cases);
+  for (auto caseOp : cases)
+    terminateBody(builder, caseOp.getCaseRegion(), caseOp.getLoc());
+  terminateBody(builder, swop.getBody(), swop.getLoc());
+
+  return res;
+}
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 3cd17053a52ba..85ba12d41a516 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 
+#include "clang/CIR/Dialect/IR/CIROpsEnums.h"
 #include "clang/CIR/Dialect/IR/CIRTypes.h"
 
 #include "mlir/Interfaces/ControlFlowInterfaces.h"
@@ -166,7 +167,8 @@ void cir::AllocaOp::build(mlir::OpBuilder &odsBuilder,
 
 LogicalResult cir::BreakOp::verify() {
   assert(!cir::MissingFeatures::switchOp());
-  if (!getOperation()->getParentOfType<LoopOpInterface>())
+  if (!getOperation()->getParentOfType<LoopOpInterface>() &&
+      !getOperation()->getParentOfType<SwitchOp>())
     return emitOpError("must be within a loop");
   return success();
 }
@@ -802,6 +804,132 @@ Block *cir::BrCondOp::getSuccessorForOperands(Array...
[truncated]

Andres-Salamanca · 2025-04-24T01:29:58Z

@andykaylor

xlauko · 2025-04-24T13:57:09Z

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

+  let hasVerifier = 1;
+
+  let skipDefaultBuilders = 1;
+let builders = [


xlauko · 2025-04-24T13:59:51Z

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

+// Check if the switch is in a simple form. If yes, collect the cases to \param
+// cases. This is an expensive and need to be used with caution.


This is not true as it collects cases also in the case it is not in simple form.

The comment isn't in the .cpp file anymore but still exists in the TableGen definition here

Should this be updated first in the incubator repository, or is it okay if I go ahead and change it directly here?

xlauko · 2025-04-24T14:02:20Z

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

+  insertPoint = builder.saveInsertionPoint();
+}
+
+LogicalResult cir::CaseOp::verify() { return success(); }


why empty verifier?

This verify() function was introduced when cir.case was added in this commit. Before that, cases in a cir.switch were represented as attributes and handled in the parseSwitchOp function see this version of the file.

With the introduction of cir.case, that logic was refactored. Now, cases are modeled as proper operations rather than attributes. Most of the case handling logic was moved to CIRGenFunction::buildCaseDefaultCascade and CIRGenFunction::foldCaseStmt.

From what I understand, the rationale for this change is:

"Then we can get the cases by traversing the body of the cir.switch operation easily instead of counting the regions and the attributes."

In the current setup, CIRGenFunction::emitSwitchBody is responsible for creating the cir.case ops within the body of the cir.switch, and buildCaseDefaultCascade handles the logic for constructing and verifying that the case layout is correct. @bcardosolopes is this correct, or is there something I’m missing?

My point was that it is unnecessary, since it has no use returning just success and can be removed also with let hasVerifier = 1; from operation definition. And should be added only when we have some meaningful check in the verifier.

Oh okay, done!

xlauko · 2025-04-24T14:03:04Z

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

+  cir::IntType intCondType;
+
+  if (parser.parseLParen())
+    return ::mlir::failure();


No need for ::mlir use mlir also in other places.

bcardosolopes

The overall approach looks good, it does seem like not a lot of tests for the amount of code added - switches took multiple rounds in the incubator to get to this state, I'm positive you can find more testcases in incubator to bullet proof this!

Andres-Salamanca · 2025-04-24T22:57:24Z

@bcardosolopes I've added more test, let me know if there's a specific scenario you think is still missing

xlauko · 2025-04-24T14:05:40Z

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

+  region.push_back(RegionSuccessor(&getBody()));
+}
+
+LogicalResult cir::SwitchOp::verify() { return success(); }


why empty verifier?

Same comment to meaningless verifier applies here.

xlauko · 2025-04-25T08:11:26Z

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

+  insertPoint = builder.saveInsertionPoint();
+}
+
+LogicalResult cir::CaseOp::verify() { return success(); }


My point was that it is unnecessary, since it has no use returning just success and can be removed also with let hasVerifier = 1; from operation definition. And should be added only when we have some meaningful check in the verifier.

…rage

bcardosolopes · 2025-04-25T21:35:57Z

I've added more test, let me know if there's a specific scenario you think is still missing

Thanks! Nothing specific, I just want to make sure we reflect as much as possible. Is there any switch test you could't add here because it was using other constructs not yet supported? If so I suggest you trim them down and also add them.

bcardosolopes · 2025-04-25T21:36:36Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+  // TODO: LLVM codegen does some early optimization to fold the condition and
+  // only emit live cases. CIR should use MLIR to achieve similar things,
+  // nothing to be done here.
+  // if (ConstantFoldsToSimpleInteger(S.getCond(), ConstantCondValue))...


It would be good to have an assert on missing feature to track this later on

@bcardosolopes ConstantFoldsToSimpleInteger is already implemented in the incubator here, so I believe this comment is outdated. What should I do in this case?

As long as the implementation is missing in the upstream repo, a MissingFeatures assert is appropriate.

Sorry, I forgot to mention this has already been upstreamed here.

bcardosolopes

LGTM after nits + once @xlauko is happy!

andykaylor · 2025-04-25T23:10:36Z

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

+    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


Suggested change

directly. However, when all the `cir.case` operations lives in the region

directly. However, when all the `cir.case` operations live in the region

andykaylor · 2025-04-25T23:10:58Z

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

+
+    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


Suggested change

of `cir.switch` directly and there is no other operations except the ending

of `cir.switch` directly and there are no other operations except the ending

andykaylor · 2025-04-25T23:11:26Z

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

+    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


Suggested change

`cir.yield` operation in the region of `cir.switch` directly, we call the

`cir.yield` operation in the region of `cir.switch` directly, we say the

andykaylor · 2025-04-25T23:12:11Z

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

+    `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
+    makes analysis easier to handle the `cir.switch` operation


Suggested change

makes analysis easier to handle the `cir.switch` operation

makes it easier for analyses to handle the `cir.switch` operation

andykaylor · 2025-04-25T23:12:27Z

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

+    `bool isSimpleForm(llvm::SmallVector<CaseOp> &cases)` member function to
+    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.


Suggested change

and makes the boundary to give up pretty clear.

and makes the boundary to give up clear.

andykaylor · 2025-04-25T23:52:12Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+  // simple form later since the conversion itself should be harmless.
+  if (subStmtKind == SubStmtKind::Case)
+    result = emitCaseStmt(*cast<CaseStmt>(sub), condType, buildingTopLevelCase);
+  else if (subStmtKind == SubStmtKind::Default) {


Suggested change

else if (subStmtKind == SubStmtKind::Default) {

} else if (subStmtKind == SubStmtKind::Default) {

andykaylor · 2025-04-25T23:52:41Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+    // the case we're building, then we can attach more random stmts to the
+    // case to make generating `cir.switch` operation to be a simple form.
+    builder.restoreInsertionPoint(insertPoint);
+


Suggested change

}

andykaylor · 2025-04-25T23:55:51Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+  // TODO: LLVM codegen does some early optimization to fold the condition and
+  // only emit live cases. CIR should use MLIR to achieve similar things,
+  // nothing to be done here.
+  // if (ConstantFoldsToSimpleInteger(S.getCond(), ConstantCondValue))...


As long as the implementation is missing in the upstream repo, a MissingFeatures assert is appropriate.

andykaylor · 2025-04-25T23:57:05Z

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

+  switchBuilder(builder, result.location, result);
+}
+
+void cir::SwitchOp::collectCases(llvm::SmallVector<CaseOp> &cases) {


Suggested change

void cir::SwitchOp::collectCases(llvm::SmallVector<CaseOp> &cases) {

void cir::SwitchOp::collectCases(llvm::SmallVectorImpl<CaseOp> &cases) {

andykaylor · 2025-04-25T23:57:17Z

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

+  });
+}
+
+bool cir::SwitchOp::isSimpleForm(llvm::SmallVector<CaseOp> &cases) {


Suggested change

bool cir::SwitchOp::isSimpleForm(llvm::SmallVector<CaseOp> &cases) {

bool cir::SwitchOp::isSimpleForm(llvm::SmallVectorImpl<CaseOp> &cases) {

andykaylor

Looks good to me, with a couple more nits about grammar in the comments.

andykaylor · 2025-04-28T20:49:45Z

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

+    `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`


Suggested change

The `cir.case` operations doesn't have to be in the region of `cir.switch`

The `cir.case` operations don't have to be in the region of `cir.switch`

This one too.

andykaylor · 2025-04-28T20:50:58Z

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

+    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


Suggested change

represents the semantics of a C/C++ switch statement. Users can use

represent the semantics of a C/C++ switch statement. Users can use

Sorry I missed this one in my earlier grammar corrections.

This introduces initial support for the switchOP, with caseOP of the Equal kind. Support for additional case kinds such as AnyOf, Range, and Default will be included in a future patch. Similarly, lowering to LLVM IR and flattening of the switch are deferred and will be addressed in subsequent updates.

Andres-Salamanca requested review from lanza and bcardosolopes as code owners April 24, 2025 01:28

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Apr 24, 2025

xlauko reviewed Apr 24, 2025

View reviewed changes

bcardosolopes reviewed Apr 24, 2025

View reviewed changes

andykaylor requested review from mmha, erichkeane and andykaylor April 24, 2025 21:23

xlauko reviewed Apr 25, 2025

View reviewed changes

Andres-Salamanca requested a review from bcardosolopes April 25, 2025 19:04

Andres-Salamanca force-pushed the cir-switch branch from ada9d74 to 9682077 Compare April 25, 2025 19:12

Andres-Salamanca added 6 commits April 25, 2025 14:15

Add initial CIR support for switch operation

f1f56e1

Add codegen for switch and EqualCase kind

b5d56d5

Add early return after NYI to prevent crashes

08ad256

Fixed format, addressed reviews, and added more tests for better cove…

6e1e7c3

…rage

Remove empty verifier

a8741c5

Rename tests to match with new mangled names

ad4fd87

Andres-Salamanca force-pushed the cir-switch branch from 9682077 to ad4fd87 Compare April 25, 2025 19:18

Andres-Salamanca requested a review from xlauko April 25, 2025 19:21

bcardosolopes reviewed Apr 25, 2025

View reviewed changes

bcardosolopes approved these changes Apr 25, 2025

View reviewed changes

Add more switch tests and mark unsupported constructs as TODO

7d5a53f

Andres-Salamanca requested a review from bcardosolopes April 25, 2025 22:56

andykaylor reviewed Apr 25, 2025

View reviewed changes

Apply new code review suggestions

d7dbe0c

Andres-Salamanca force-pushed the cir-switch branch from 8f0eb90 to d7dbe0c Compare April 28, 2025 18:28

Andres-Salamanca requested a review from andykaylor April 28, 2025 18:30

andykaylor approved these changes Apr 28, 2025

View reviewed changes

Grammar corrections

25aabcd

andykaylor merged commit 9d1f1c4 into llvm:main Apr 29, 2025
11 checks passed

Andres-Salamanca deleted the cir-switch branch May 7, 2025 21:32

		// Check if the switch is in a simple form. If yes, collect the cases to \param
		// cases. This is an expensive and need to be used with caution.

	directly. However, when all the `cir.case` operations lives in the region
	directly. However, when all the `cir.case` operations live in the region

	of `cir.switch` directly and there is no other operations except the ending
	of `cir.switch` directly and there are no other operations except the ending

	`cir.yield` operation in the region of `cir.switch` directly, we call the
	`cir.yield` operation in the region of `cir.switch` directly, we say the

	makes analysis easier to handle the `cir.switch` operation
	makes it easier for analyses to handle the `cir.switch` operation

	and makes the boundary to give up pretty clear.
	and makes the boundary to give up clear.

	else if (subStmtKind == SubStmtKind::Default) {
	} else if (subStmtKind == SubStmtKind::Default) {

	void cir::SwitchOp::collectCases(llvm::SmallVector<CaseOp> &cases) {
	void cir::SwitchOp::collectCases(llvm::SmallVectorImpl<CaseOp> &cases) {

	bool cir::SwitchOp::isSimpleForm(llvm::SmallVector<CaseOp> &cases) {
	bool cir::SwitchOp::isSimpleForm(llvm::SmallVectorImpl<CaseOp> &cases) {

	The `cir.case` operations doesn't have to be in the region of `cir.switch`
	The `cir.case` operations don't have to be in the region of `cir.switch`

	represents the semantics of a C/C++ switch statement. Users can use
	represent the semantics of a C/C++ switch statement. Users can use

[CIR] Upstream initial support for switch statements #137106

[CIR] Upstream initial support for switch statements #137106

Conversation

Andres-Salamanca commented Apr 24, 2025

llvmbot commented Apr 24, 2025 • edited Loading

Andres-Salamanca commented Apr 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes left a comment

Choose a reason for hiding this comment

Andres-Salamanca commented Apr 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes commented Apr 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andykaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Apr 24, 2025 •

edited

Loading