[CIR] Upstream support for switch statements case kinds #138003

Andres-Salamanca · 2025-04-30T17:57:01Z

This introduces support for the following cir::case kinds:

Equal
AnyOf
Range

llvmbot · 2025-04-30T17:57:34Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: None (Andres-Salamanca)

Changes

This introduces support for the following cir::case kinds:

Equal
AnyOf
Range

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

4 Files Affected:

(modified) clang/include/clang/CIR/MissingFeatures.h (-1)
(modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+8)
(modified) clang/lib/CIR/CodeGen/CIRGenStmt.cpp (+65-16)
(modified) clang/test/CIR/CodeGen/switch.cpp (+236-11)

diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 4d4951aa0e126..8f58e10d9070e 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -161,7 +161,6 @@ struct MissingFeatures {
   static bool targetSpecificCXXABI() { return false; }
   static bool moduleNameHash() { return false; }
   static bool setDSOLocal() { return false; }
-  static bool foldCaseStmt() { return false; }
   static bool constantFoldSwitchStatement() { return false; }
 
   // Missing types
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 592d39930089d..fb1a7dc75161d 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -278,6 +278,9 @@ class CIRGenFunction : public CIRGenTypeCache {
   /// addressed later.
   RValue getUndefRValue(clang::QualType ty);
 
+  const CaseStmt *foldCaseStmt(const clang::CaseStmt &s, mlir::Type condType,
+                               mlir::ArrayAttr &value, cir::CaseOpKind &kind);
+
   cir::FuncOp generateCode(clang::GlobalDecl gd, cir::FuncOp fn,
                            cir::FuncType funcType);
 
@@ -532,6 +535,11 @@ class CIRGenFunction : public CIRGenTypeCache {
   mlir::LogicalResult emitDeclStmt(const clang::DeclStmt &s);
   LValue emitDeclRefLValue(const clang::DeclRefExpr *e);
 
+
+  mlir::LogicalResult emitDefaultStmt(const clang::DefaultStmt &s,
+                                      mlir::Type condType,
+                                      bool buildingTopLevelCase);
+
   /// Emit an `if` on a boolean condition to the specified blocks.
   /// FIXME: Based on the condition, this might try to simplify the codegen of
   /// the conditional based on the branch.
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index 31e29e7828156..56e9ba04c8ce0 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -253,6 +253,7 @@ mlir::LogicalResult CIRGenFunction::emitSimpleStmt(const Stmt *s,
   case Stmt::NullStmtClass:
     break;
   case Stmt::CaseStmtClass:
+  case Stmt::DefaultStmtClass:
     // If we reached here, we must not handling a switch case in the top level.
     return emitSwitchCase(cast<SwitchCase>(*s),
                           /*buildingTopLevelCase=*/false);
@@ -428,6 +429,53 @@ mlir::LogicalResult CIRGenFunction::emitBreakStmt(const clang::BreakStmt &s) {
   return mlir::success();
 }
 
+
+const CaseStmt *CIRGenFunction::foldCaseStmt(const clang::CaseStmt &s,
+                                             mlir::Type condType,
+                                             mlir::ArrayAttr &value,
+                                             cir::CaseOpKind &kind) {
+  const CaseStmt *caseStmt = &s;
+  const CaseStmt *lastCase = &s;
+  SmallVector<mlir::Attribute, 4> caseEltValueListAttr;
+
+  // Fold cascading cases whenever possible to simplify codegen a bit.
+  while (caseStmt) {
+    lastCase = caseStmt;
+
+    auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());
+
+    if (auto *rhs = caseStmt->getRHS()) {
+      auto endVal = rhs->EvaluateKnownConstInt(getContext());
+      SmallVector<mlir::Attribute, 4> rangeCaseAttr = {
+          cir::IntAttr::get(condType, intVal),
+          cir::IntAttr::get(condType, endVal)};
+      value = builder.getArrayAttr(rangeCaseAttr);
+      kind = cir::CaseOpKind::Range;
+
+      // We may not be able to fold rangaes. Due to we can't present range case
+      // with other trivial cases now.
+      return caseStmt;
+    }
+
+    caseEltValueListAttr.push_back(cir::IntAttr::get(condType, intVal));
+
+    caseStmt = dyn_cast_or_null<CaseStmt>(caseStmt->getSubStmt());
+
+    // Break early if we found ranges. We can't fold ranges due to the same
+    // reason above.
+    if (caseStmt && caseStmt->getRHS())
+      break;
+  }
+
+  if (!caseEltValueListAttr.empty()) {
+    value = builder.getArrayAttr(caseEltValueListAttr);
+    kind = caseEltValueListAttr.size() > 1 ? cir::CaseOpKind::Anyof
+                                           : cir::CaseOpKind::Equal;
+  }
+
+  return lastCase;
+}
+
 template <typename T>
 mlir::LogicalResult
 CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
@@ -500,8 +548,8 @@ CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
   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();
+    result = emitDefaultStmt(*cast<DefaultStmt>(sub), condType,
+                             buildingTopLevelCase);
   } 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
@@ -515,19 +563,21 @@ CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
 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,
+  cir::CaseOpKind kind;
+  mlir::ArrayAttr value;
+  const CaseStmt *caseStmt = foldCaseStmt(s, condType, value, kind);
+  return emitCaseDefaultCascade(caseStmt, condType, value, kind,
                                 buildingTopLevelCase);
 }
 
+
+  mlir::LogicalResult CIRGenFunction::emitDefaultStmt(const clang::DefaultStmt &s,
+                                      mlir::Type condType,
+                                      bool buildingTopLevelCase) {
+  return emitCaseDefaultCascade(&s, condType, builder.getArrayAttr({}),
+                                cir::CaseOpKind::Default, buildingTopLevelCase);
+}
+
 mlir::LogicalResult CIRGenFunction::emitSwitchCase(const SwitchCase &s,
                                                    bool buildingTopLevelCase) {
   assert(!condTypeStack.empty() &&
@@ -537,10 +587,9 @@ mlir::LogicalResult CIRGenFunction::emitSwitchCase(const SwitchCase &s,
     return emitCaseStmt(cast<CaseStmt>(s), condTypeStack.back(),
                         buildingTopLevelCase);
 
-  if (s.getStmtClass() == Stmt::DefaultStmtClass) {
-    getCIRGenModule().errorNYI(s.getSourceRange(), "Default case");
-    return mlir::failure();
-  }
+  if (s.getStmtClass() == Stmt::DefaultStmtClass)
+    return emitDefaultStmt(cast<DefaultStmt>(s), condTypeStack.back(),
+                           buildingTopLevelCase);
 
   llvm_unreachable("expect case or default stmt");
 }
diff --git a/clang/test/CIR/CodeGen/switch.cpp b/clang/test/CIR/CodeGen/switch.cpp
index 36523755376a1..ea0e95e7eb553 100644
--- a/clang/test/CIR/CodeGen/switch.cpp
+++ b/clang/test/CIR/CodeGen/switch.cpp
@@ -16,6 +16,7 @@ void sw1(int a) {
   }
   }
 }
+
 // CIR: cir.func @_Z3sw1i
 // CIR: cir.switch (%3 : !s32i) {
 // CIR-NEXT: cir.case(equal, [#cir.int<0> : !s32i]) {
@@ -91,12 +92,40 @@ void sw2(int a) {
 // OGCG: [[SW_EPILOG]]:
 // OGCG:   ret void
 
+void sw3(int a) {
+  switch (a) {
+  default:
+    break;
+  }
+}
+
+// CIR: cir.func @_Z3sw3i
+// CIR: cir.scope {
+// CIR-NEXT:   %1 = cir.load %0 : !cir.ptr<!s32i>, !s32i
+// CIR-NEXT:   cir.switch (%1 : !s32i) {
+// CIR-NEXT:   cir.case(default, []) {
+// CIR-NEXT:     cir.break
+// CIR-NEXT:   }
+// CIR-NEXT:   cir.yield
+// CIR-NEXT:   }
+
+// OGCG: define dso_local void @_Z3sw3i
+// OGCG: entry:
+// OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
+// OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
+// OGCG:   switch i32 %[[A_VAL]], label %[[DEFAULT:.*]] [
+// OGCG: [[DEFAULT]]:
+// OGCG:   br label %[[EPILOG:.*]]
+// OGCG: [[EPILOG]]:
+// OGCG:   ret void
+
 int sw4(int a) {
   switch (a) {
   case 42: {
     return 3;
   }
-  // TODO: add default case when it is upstreamed
+  default:
+    return 2;
   }
   return 0;
 }
@@ -112,24 +141,31 @@ int sw4(int a) {
 // CIR-NEXT:         }
 // CIR-NEXT:         cir.yield
 // CIR-NEXT:       }
+// CIR-NEXT:       cir.case(default, []) {
+// CIR-NEXT:         %5 = cir.const #cir.int<2> : !s32i
+// CIR-NEXT:         cir.store %5, %1 : !s32i, !cir.ptr<!s32i>
+// CIR-NEXT:         %6 = cir.load %1 : !cir.ptr<!s32i>, !s32i
+// CIR-NEXT:         cir.return %6 : !s32i
+// CIR-NEXT:       }
+// CIR-NEXT:       cir.yield
+// CIR-NEXT:       }
 
 // OGCG: define dso_local noundef i32 @_Z3sw4i
 // OGCG: entry:
 // OGCG:   %[[RETVAL:.*]] = alloca i32, align 4
 // OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
 // OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
-// OGCG:   switch i32 %[[A_VAL]], label %[[EPILOG:.*]] [
+// OGCG:   switch i32 %[[A_VAL]], label %[[DEFAULT:.*]] [
 // OGCG:     i32 42, label %[[SW42:.*]]
 // OGCG:   ]
 // OGCG: [[SW42]]:
 // OGCG:   br label %[[RETURN:.*]]
-// OGCG: [[EPILOG]]:
+// OGCG: [[DEFAULT]]:
 // OGCG:   br label %[[RETURN]]
 // OGCG: [[RETURN]]:
 // OGCG:   %[[RETVAL_LOAD:.*]] = load i32, ptr %[[RETVAL]], align 4
 // OGCG:   ret i32 %[[RETVAL_LOAD]]
 
-
 void sw5(int a) {
   switch (a) {
   case 1:;
@@ -156,13 +192,97 @@ void sw5(int a) {
 // OGCG: [[SW_EPILOG]]:
 // OGCG:   ret void
 
+void sw6(int a) {
+  switch (a) {
+  case 0:
+  case 1:
+  case 2:
+    break;
+  case 3:
+  case 4:
+  case 5:
+    break;
+  }
+}
+
+// CIR: cir.func @_Z3sw6i
+// CIR: cir.switch (%1 : !s32i) {
+// CIR-NEXT: cir.case(anyof, [#cir.int<0> : !s32i, #cir.int<1> : !s32i, #cir.int<2> : !s32i]) {
+// CIR-NEXT:   cir.break
+// CIR-NEXT: }
+// CIR-NEXT: cir.case(anyof, [#cir.int<3> : !s32i, #cir.int<4> : !s32i, #cir.int<5> : !s32i]) {
+// CIR-NEXT:   cir.break
+// CIR-NEXT: }
+
+// OGCG: define dso_local void @_Z3sw6i
+// OGCG: entry:
+// OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
+// OGCG:   store i32 %a, ptr %[[A_ADDR]], align 4
+// OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
+// OGCG:   switch i32 %[[A_VAL]], label %[[EPILOG:.*]] [
+// OGCG:     i32 0, label %[[BB0:.*]]
+// OGCG:     i32 1, label %[[BB0]]
+// OGCG:     i32 2, label %[[BB0]]
+// OGCG:     i32 3, label %[[BB1:.*]]
+// OGCG:     i32 4, label %[[BB1]]
+// OGCG:     i32 5, label %[[BB1]]
+// OGCG:   ]
+// OGCG: [[BB0]]:
+// OGCG:   br label %[[EPILOG]]
+// OGCG: [[BB1]]:
+// OGCG:   br label %[[EPILOG]]
+// OGCG: [[EPILOG]]:
+// OGCG:   ret void
+
+void sw7(int a) {
+  switch (a) {
+  case 0:
+  case 1:
+  case 2:
+    int x;
+  case 3:
+  case 4:
+  case 5:
+    break;
+  }
+}
+
+// CIR: cir.func @_Z3sw7i
+// CIR: cir.case(anyof, [#cir.int<0> : !s32i, #cir.int<1> : !s32i, #cir.int<2> : !s32i]) {
+// CIR-NEXT:   cir.yield
+// CIR-NEXT: }
+// CIR-NEXT: cir.case(anyof, [#cir.int<3> : !s32i, #cir.int<4> : !s32i, #cir.int<5> : !s32i]) {
+// CIR-NEXT:   cir.break
+// CIR-NEXT: }
+
+
+// OGCG: define dso_local void @_Z3sw7i
+// OGCG: entry:
+// OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
+// OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
+// OGCG:   switch i32 %[[A_VAL]], label %[[EPILOG:.*]] [
+// OGCG:     i32 0, label %[[BB0:.*]]
+// OGCG:     i32 1, label %[[BB0]]
+// OGCG:     i32 2, label %[[BB0]]
+// OGCG:     i32 3, label %[[BB1:.*]]
+// OGCG:     i32 4, label %[[BB1]]
+// OGCG:     i32 5, label %[[BB1]]
+// OGCG:   ]
+// OGCG: [[BB0]]:
+// OGCG:   br label %[[BB1]]
+// OGCG: [[BB1]]:
+// OGCG:   br label %[[EPILOG]]
+// OGCG: [[EPILOG]]:
+// OGCG:   ret void
+
+
 void sw8(int a) {
   switch (a)
   {
   case 3:
     break;
   case 4:
-  // TODO: add default case when it is upstreamed
+  default:
     break;
   }
 }
@@ -172,6 +292,9 @@ void sw8(int a) {
 // CIR-NEXT:   cir.break
 // CIR-NEXT: }
 // CIR-NEXT: cir.case(equal, [#cir.int<4> : !s32i]) {
+// CIR-NEXT:   cir.yield
+// CIR-NEXT: }
+// CIR-NEXT: cir.case(default, []) {
 // CIR-NEXT:   cir.break
 // CIR-NEXT: }
 
@@ -180,24 +303,25 @@ void sw8(int a) {
 // OGCG: entry:
 // OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
 // OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
-// OGCG:   switch i32 %[[A_VAL]], label %[[EPILOG:.*]] [
+// OGCG:   switch i32 %[[A_VAL]], label %[[DEFAULT:.*]] [
 // OGCG:     i32 3, label %[[SW3:.*]]
 // OGCG:     i32 4, label %[[SW4:.*]]
 // OGCG:   ]
 // OGCG: [[SW3]]:
-// OGCG:   br label %[[EPILOG]]
+// OGCG:   br label %[[EPILOG:.*]]
 // OGCG: [[SW4]]:
+// OGCG:   br label %[[DEFAULT]]
+// OGCG: [[DEFAULT]]:
 // OGCG:   br label %[[EPILOG]]
 // OGCG: [[EPILOG]]:
 // OGCG:   ret void
 
-
 void sw9(int a) {
   switch (a)
   {
   case 3:
     break;
-  // TODO: add default case when it is upstreamed
+  default:
   case 4:
     break;
   }
@@ -207,6 +331,9 @@ void sw9(int a) {
 // CIR:      cir.case(equal, [#cir.int<3> : !s32i]) {
 // CIR-NEXT:   cir.break
 // CIR-NEXT: }
+// CIR-NEXT: cir.case(default, []) {
+// CIR-NEXT:   cir.yield
+// CIR-NEXT: }
 // CIR-NEXT: cir.case(equal, [#cir.int<4> : !s32i]) {
 // CIR-NEXT:   cir.break
 // CIR-NEXT: }
@@ -215,17 +342,115 @@ void sw9(int a) {
 // OGCG: entry:
 // OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
 // OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
-// OGCG:   switch i32 %[[A_VAL]], label %[[EPILOG:.*]] [
+// OGCG:   switch i32 %[[A_VAL]], label %[[DEFAULT:.*]] [
 // OGCG:     i32 3, label %[[SW3:.*]]
 // OGCG:     i32 4, label %[[SW4:.*]]
 // OGCG:   ]
 // OGCG: [[SW3]]:
-// OGCG:   br label %[[EPILOG]]
+// OGCG:   br label %[[EPILOG:.*]]
+// OGCG: [[DEFAULT]]:
+// OGCG:   br label %[[SW4]]
 // OGCG: [[SW4]]:
 // OGCG:   br label %[[EPILOG]]
 // OGCG: [[EPILOG]]:
 // OGCG:   ret void
 
+void sw10(int a) {
+  switch (a)
+  {
+  case 3:
+    break;
+  case 4:
+  default:
+  case 5:
+    break;
+  }
+}
+
+//CIR:    cir.func @_Z4sw10i
+//CIR:      cir.case(equal, [#cir.int<3> : !s32i]) {
+//CIR-NEXT:   cir.break
+//CIR-NEXT: }
+//CIR-NEXT: cir.case(equal, [#cir.int<4> : !s32i]) {
+//CIR-NEXT:   cir.yield
+//CIR-NEXT: }
+//CIR-NEXT: cir.case(default, []) {
+//CIR-NEXT:   cir.yield
+//CIR-NEXT: }
+//CIR-NEXT: cir.case(equal, [#cir.int<5> : !s32i]) {
+//CIR-NEXT:   cir.break
+//CIR-NEXT: }
+
+// OGCG: define dso_local void @_Z4sw10i
+// OGCG: entry:
+// OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
+// OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
+// OGCG:   switch i32 %[[A_VAL]], label %[[DEFAULT:.*]] [
+// OGCG:     i32 3, label %[[BB3:.*]]
+// OGCG:     i32 4, label %[[BB4:.*]]
+// OGCG:     i32 5, label %[[BB5:.*]]
+// OGCG:   ]
+// OGCG: [[BB3]]:
+// OGCG:   br label %[[EPILOG:.*]]
+// OGCG: [[BB4]]:
+// OGCG:   br label %[[DEFAULT]]
+// OGCG: [[DEFAULT]]:
+// OGCG:   br label %[[BB5]]
+// OGCG: [[BB5]]:
+// OGCG:   br label %[[EPILOG]]
+// OGCG: [[EPILOG]]:
+// OGCG:   ret void
+
+void sw11(int a) {
+  switch (a)
+  {
+  case 3:
+    break;
+  case 4:
+  case 5:
+  default:
+  case 6:
+  case 7:
+    break;
+  }
+}
+
+//CIR:    cir.func @_Z4sw11i
+//CIR:      cir.case(equal, [#cir.int<3> : !s32i]) {
+//CIR-NEXT:   cir.break
+//CIR-NEXT: }
+//CIR-NEXT: cir.case(anyof, [#cir.int<4> : !s32i, #cir.int<5> : !s32i]) {
+//CIR-NEXT:   cir.yield
+//CIR-NEXT: }
+//CIR-NEXT: cir.case(default, []) {
+//CIR-NEXT:   cir.yield
+//CIR-NEXT: }
+//CIR-NEXT: cir.case(anyof, [#cir.int<6> : !s32i, #cir.int<7> : !s32i]) {
+//CIR-NEXT:   cir.break
+//CIR-NEXT: }
+
+// OGCG: define dso_local void @_Z4sw11i
+// OGCG: entry:
+// OGCG:   %[[A_ADDR:.*]] = alloca i32, align 4
+// OGCG:   %[[A_VAL:.*]] = load i32, ptr %[[A_ADDR]], align 4
+// OGCG:   switch i32 %[[A_VAL]], label %[[DEFAULT:.*]] [
+// OGCG:     i32 3, label %[[BB3:.*]]
+// OGCG:     i32 4, label %[[BB4:.*]]
+// OGCG:     i32 5, label %[[BB4]]
+// OGCG:     i32 6, label %[[BB6:.*]]
+// OGCG:     i32 7, label %[[BB6]]
+// OGCG:   ]
+// OGCG: [[BB3]]:
+// OGCG:   br label %[[EPILOG:.*]]
+// OGCG: [[BB4]]:
+// OGCG:   br label %[[DEFAULT]]
+// OGCG: [[DEFAULT]]:
+// OGCG:   br label %[[BB6]]
+// OGCG: [[BB6]]:
+// OGCG:   br label %[[EPILOG]]
+// OGCG: [[EPILOG]]:
+// OGCG:   ret void
+
 void sw12(int a) {
   switch (a)
   {

github-actions · 2025-04-30T17:59:42Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Andres-Salamanca · 2025-04-30T19:05:00Z

@andykaylor

bcardosolopes · 2025-04-30T21:45:45Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+      value = builder.getArrayAttr(rangeCaseAttr);
+      kind = cir::CaseOpKind::Range;
+
+      // We may not be able to fold rangaes. Due to we can't present range case


rangaes -> ranges

bcardosolopes · 2025-04-30T21:46:15Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+      kind = cir::CaseOpKind::Range;
+
+      // We may not be able to fold rangaes. Due to we can't present range case
+      // with other trivial cases now.


Sounds like it deserves an assert on missing features?

erichkeane · 2025-05-01T17:31:13Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

@@ -253,6 +253,7 @@ mlir::LogicalResult CIRGenFunction::emitSimpleStmt(const Stmt *s,
  case Stmt::NullStmtClass:
    break;
  case Stmt::CaseStmtClass:
+  case Stmt::DefaultStmtClass:


What does 'top level' switch case mean here? I realize it is pre-existing, but trying to grok what is going on here

erichkeane · 2025-05-01T17:33:09Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+  while (caseStmt) {
+    lastCase = caseStmt;
+
+    auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());


Don't use auto here, or 447.

erichkeane · 2025-05-01T17:34:54Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+
+    auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());
+
+    if (auto *rhs = caseStmt->getRHS()) {


RHS is only valid for a GNU-range version of a case, do we have a test for that?

erichkeane · 2025-05-01T17:36:17Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+
+  if (!caseEltValueListAttr.empty()) {
+    value = builder.getArrayAttr(caseEltValueListAttr);
+    kind = caseEltValueListAttr.size() > 1 ? cir::CaseOpKind::Anyof


So IIRC, 'anyof' is only valid if the case statements themselves are empty, right? Else they could contain a label for a GOTO, or be a duffs-device/etc. So i think we're being overly aggressive about joining these up here.

That said, i find myself wondering if the FE should be doing this sort of joining at all, rather than as a very early 'normalization' opt-pass.

I agree that this should be moved to an optimization pass. Perhaps CIRSimplify?

cir-simplify isn't upstreamed yet should I go ahead and create it ?

I would prefer we do it in a followup, but it might be nice to see that sort of thing happen.

Just to confirm in this case, should I leave the current code and add a TODO for the follow-up work?
Because if we move this logic to a pass , that means the tests will need to change as well, since the anyof won't be emitted until after the simplify is upstreamed.

I would prefer to just change the tests. The simplify pass shouldn't be run for the emit-cir tests unless it's explicitly added. There are a number of cases where we're doing optimization like this in the front end, and I really don't think we should be.

FYI, @mmha has been working on cir-simplify and will be posting a PR soon, so you should wait for that to land and then add the case folding after it is in place.

If we're no longer folding cascading case statements, then this function could be simplified we wouldn’t need the while loop anymore since we're just extracting the kind and value from a single case. In that scenario, the assertion proposed by @andykaylor would no longer be necessary, because range cases would only ever be processed once at the top level.

const CaseStmt *CIRGenFunction::foldCaseStmt(const clang::CaseStmt &s, mlir::Type condType, mlir::ArrayAttr &value, cir::CaseOpKind &kind) { const CaseStmt *caseStmt = &s; SmallVector<mlir::Attribute, 1> caseEltValueListAttr; llvm::APSInt intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext()); // If the case statement has an RHS value, it is representing a GNU // case range statement, where LHS is the beginning of the range // and RHS is the end of the range. if (const Expr *rhs = caseStmt->getRHS()) { assert(caseStmt == &s && "Range case must be the first case processed"); llvm::APSInt endVal = rhs->EvaluateKnownConstInt(getContext()); SmallVector<mlir::Attribute, 4> rangeCaseAttr = { cir::IntAttr::get(condType, intVal), cir::IntAttr::get(condType, endVal)}; value = builder.getArrayAttr(rangeCaseAttr); kind = cir::CaseOpKind::Range; // We don't currently fold case range statements with other case statements. // TODO(cir): Add this capability. assert(!cir::MissingFeatures::foldRangeCase()); return caseStmt; } caseEltValueListAttr.push_back(cir::IntAttr::get(condType, intVal)); if (!caseEltValueListAttr.empty()) { value = builder.getArrayAttr(caseEltValueListAttr); kind = cir::CaseOpKind::Equal; } return caseStmt; }

We don’t need the while loop anymore because we must break as soon as we encounter a cascading case or default statement.

// Break early if we found a range. We can't fold ranges. // Also break if we found a cascading case/default. if (caseStmt) { const Stmt *sub = caseStmt->getSubStmt(); if (caseStmt->getRHS() || isa<CaseStmt>(sub) || isa<DefaultStmt>(sub)) break; }

Also, given that we're no longer folding multiple cases, I think we should consider renaming the function to better reflect its new behavior. Something like getCaseInfo ?
Is my approach correct ?

I would say you don't even need to call a function at all. You can go back to what you had before in emitCaseStmt but with special handling added for the GNU range case.

andykaylor · 2025-05-01T17:16:30Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+  while (caseStmt) {
+    lastCase = caseStmt;
+
+    auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());


Suggested change

auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());

APSInt intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());

andykaylor · 2025-05-01T17:17:17Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+
+    auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());
+
+    if (auto *rhs = caseStmt->getRHS()) {


Suggested change

if (auto *rhs = caseStmt->getRHS()) {

// If the case statement has an RHS value, it is representing a GNU

// case range statement, where LHS is the beginning of the range

// and RHS is the end of the range.

if (const Expr *rhs = caseStmt->getRHS()) {

andykaylor · 2025-05-01T17:17:41Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+    auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());
+
+    if (auto *rhs = caseStmt->getRHS()) {
+      auto endVal = rhs->EvaluateKnownConstInt(getContext());


Suggested change

auto endVal = rhs->EvaluateKnownConstInt(getContext());

APSInt endVal = rhs->EvaluateKnownConstInt(getContext());

andykaylor · 2025-05-01T17:33:21Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+      value = builder.getArrayAttr(rangeCaseAttr);
+      kind = cir::CaseOpKind::Range;
+
+      // We may not be able to fold rangaes. Due to we can't present range case


Suggested change

// We may not be able to fold rangaes. Due to we can't present range case

// We don't currently fold case range statements with other case statements.

// TODO(cir): Add this capability.

andykaylor · 2025-05-01T18:05:11Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+
+      // We may not be able to fold rangaes. Due to we can't present range case
+      // with other trivial cases now.
+      return caseStmt;


It took me a while to figure this out, but the check on line 465 guarantees that we will never get here unless this was the first case we are trying to fold. Can you add an assertion that verifies that? If the code is ever changed to make this untrue, we could easily lose cases.

andykaylor · 2025-05-01T18:07:17Z

clang/test/CIR/CodeGen/switch.cpp

@@ -91,12 +92,40 @@ void sw2(int a) {
 // OGCG: [[SW_EPILOG]]:
 // OGCG:   ret void

+void sw3(int a) {


Please add tests for the GNU range case, including one where it would be in the same group with other cases, like this:

void f(int x) { switch (x) { case 1: case 2: case 3 ... 6: case 7: break; default: break; } }

andykaylor · 2025-05-01T18:10:13Z

clang/test/CIR/CodeGen/switch.cpp

+// OGCG:   br label %[[EPILOG]]
+// OGCG: [[EPILOG]]:
+// OGCG:   ret void
+


Can you add a test that has non-break statements between cases?

void f(int x) { int y; switch (x) { case 1: case 2: y = 0; case 3: break; default: break; } }

…ccordingly

erichkeane · 2025-05-05T13:27:49Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

@@ -455,7 +456,8 @@ CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
    if (isa<DefaultStmt>(sub) && isa<CaseStmt>(stmt)) {
      subStmtKind = SubStmtKind::Default;
      builder.createYield(loc);
-    } else if (isa<CaseStmt>(sub) && isa<DefaultStmt>(stmt)) {
+    } else if ((isa<CaseStmt>(sub) && isa<DefaultStmt>(stmt)) ||


how about:

else if (isa<CaseStmt>(sub) && isa<CsaeStmt,DefaultStmt>(stmt)) ?

When we find a cascading case like case -> case, we emit a yield to indicate a fallthrough.

The same goes for case -> default. When we find a cascading case like case -> case or case -> default, we emit a yield to indicate fallthrough.
Here's an example of that case:

llvm-project/clang/lib/CIR/CodeGen/CIRGenStmt.cpp

Line 480 in 44856d9

// we prefer generating

Right, I get that. I'm suggesting a better alternative suggestion for this and the next line.

Oh, okay I’ll change it

erichkeane · 2025-05-05T13:28:40Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+
+    // We don't currently fold case range statements with other case statements.
+    // TODO(cir): Add this capability.
+    assert(!cir::MissingFeatures::foldRangeCase());


Do we need this? I thought we were going to move all of this to an opt pass instead?

I left it there as a reminder to fold the ranges in the future, but you're right it's not appropriate in this spot. Is there a better place to leave the comment instead?

I don't have a good idea? perhaps in the pass manager or whatever pass we expect to do this?

I updated the comment to mention implementing this in CIRSimplify.

andykaylor · 2025-05-05T18:30:42Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+  // case range statement, where LHS is the beginning of the range
+  // and RHS is the end of the range.
+  if (const Expr *rhs = s.getRHS()) {
+


Nit: I'd prefer not to have this empty line here.

andykaylor · 2025-05-05T18:39:45Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+    assert(!cir::MissingFeatures::foldCascadingCases());
+  } else {
+    caseEltValueListAttr.push_back(cir::IntAttr::get(condType, intVal));
+    value = builder.getArrayAttr(caseEltValueListAttr);


I think you can do this to eliminate the caseEltValueListAttr variable.

value = builder.getArrayAttr({cir::IntAttr::get(condType, intVal)});

andykaylor · 2025-05-05T18:40:24Z

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

+    SmallVector<mlir::Attribute, 4> rangeCaseAttr = {
+        cir::IntAttr::get(condType, intVal),
+        cir::IntAttr::get(condType, endVal)};
+    value = builder.getArrayAttr(rangeCaseAttr);


Suggested change

value = builder.getArrayAttr(rangeCaseAttr);

value = builder.getArrayAttr({

cir::IntAttr::get(condType, intVal),

cir::IntAttr::get(condType, endVal)

});

andykaylor · 2025-05-05T18:46:31Z

clang/test/CIR/CodeGen/switch.cpp

+// CIR-NEXT:         cir.return %6 : !s32i
+// CIR-NEXT:       }
+// CIR-NEXT:       cir.yield
+// CIR-NEXT:       }


Is the indentation level wrong here? This yield is terminating the cir.switch on line 134, right?

andykaylor · 2025-05-05T18:47:12Z

clang/test/CIR/CodeGen/switch.cpp

@@ -112,24 +141,31 @@ int sw4(int a) {
 // CIR-NEXT:         }
 // CIR-NEXT:         cir.yield
 // CIR-NEXT:       }
+// CIR-NEXT:       cir.case(default, []) {
+// CIR-NEXT:         %5 = cir.const #cir.int<2> : !s32i


Please update this test (throughout the file) to use pattern matching in place of the hard-coded identifiers.

andykaylor · 2025-05-05T18:52:22Z

clang/test/CIR/CodeGen/switch.cpp

+// CIR-NEXT: cir.case(equal, [#cir.int<2> : !s32i]) {
+// CIR-NEXT:     cir.yield
+// CIR-NEXT: }
+// CIR-NEXT: cir.case(equal, [#cir.int<3> : !s32i]) {


What happened to the x declaration?

It is being emitted before the switch statement:

%1 = cir.alloca !s32i, !cir.ptr<!s32i>, ["x"] {alignment = 4 : i64} %2 = cir.load %0 : !cir.ptr<!s32i>, !s32i cir.switch (%2 : !s32i) { cir.case(equal, [#cir.int<0> : !s32i]) { cir.yield ...

andykaylor · 2025-05-05T18:54:15Z

clang/test/CIR/CodeGen/switch.cpp

+}
+
+// CIR: cir.func @_Z3sw7i
+// CIR: cir.case(equal, [#cir.int<0> : !s32i]) {


The checks should include the cir.switch and any cir.scope associated with it.

…ariable

Andres-Salamanca added 2 commits April 30, 2025 12:17

add support for anyOf, default, and range case kinds

ff564e6

Remove foldcase from missing features

ee0b0aa

Andres-Salamanca requested review from lanza and bcardosolopes as code owners April 30, 2025 17:57

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

Andres-Salamanca added 2 commits April 30, 2025 13:16

Fix code formatting

3142924

Fix code formatting

b032de7

bcardosolopes reviewed Apr 30, 2025

View reviewed changes

andykaylor requested review from erichkeane and andykaylor May 1, 2025 17:10

erichkeane reviewed May 1, 2025

View reviewed changes

andykaylor reviewed May 1, 2025

View reviewed changes

Remove early optimization for cascading cases and update test cases a…

39861dd

…ccordingly

erichkeane reviewed May 5, 2025

View reviewed changes

Add foldCascadingCases to MissingFeatures and update case handling

c772736

Andres-Salamanca requested review from bcardosolopes, erichkeane and andykaylor May 5, 2025 17:53

remove foldCaseStmt

906e697

andykaylor reviewed May 5, 2025

View reviewed changes

Updated test with pattern matching and removed caseEltValueListAttr v…

f470023

…ariable

Andres-Salamanca requested a review from andykaylor May 5, 2025 20:35


		auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());

		if (auto *rhs = caseStmt->getRHS()) {

	auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());
	APSInt intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());

	auto endVal = rhs->EvaluateKnownConstInt(getContext());
	APSInt endVal = rhs->EvaluateKnownConstInt(getContext());

	// We may not be able to fold rangaes. Due to we can't present range case
	// We don't currently fold case range statements with other case statements.
	// TODO(cir): Add this capability.

[CIR] Upstream support for switch statements case kinds #138003

Are you sure you want to change the base?

[CIR] Upstream support for switch statements case kinds #138003

Conversation

Andres-Salamanca commented Apr 30, 2025

llvmbot commented Apr 30, 2025 • edited Loading

github-actions bot commented Apr 30, 2025 • edited Loading

Andres-Salamanca commented Apr 30, 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

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

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

llvmbot commented Apr 30, 2025 •

edited

Loading

github-actions bot commented Apr 30, 2025 •

edited

Loading