Apply new code review suggestions

Author: Andres-Salamanca

clang/include/clang/CIR/Dialect/IR/CIRDialect.h

@@ -59,6 +59,8 @@ class SameFirstOperandAndResultType
 
 using BuilderCallbackRef =
     llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>;
+using BuilderOpStateCallbackRef = llvm::function_ref<void(
+    mlir::OpBuilder &, mlir::Location, mlir::OperationState &)>;
 
 namespace cir {
 void buildTerminatedBody(mlir::OpBuilder &builder, mlir::Location loc);

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

@@ -821,14 +821,14 @@ def SwitchOp : CIR_Op<"switch",
     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
+    directly. However, when all the `cir.case` operations live in the region
+    of `cir.switch` directly and there are no other operations except the ending
+    `cir.yield` operation in the region of `cir.switch` directly, we say 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.
+    makes it easier for analyses to handle the `cir.switch` operation
+    and makes the boundary to give up clear.
 
     To make the simple form as common as possible, CIR code generation attaches
     operations corresponding to the statements that lives between top level
@@ -840,9 +840,8 @@ def SwitchOp : CIR_Op<"switch",
     switch(int cond) {
       case 4:
         a++;
-
-      b++;
-      case 5;
+        b++;
+      case 5:
         c++;
 
       ...
@@ -877,7 +876,7 @@ def SwitchOp : CIR_Op<"switch",
     switch(int cond) {
       case 4:
       default;
-      case 5;
+      case 5:
         a++;
       ...
     }
@@ -901,13 +900,13 @@ def SwitchOp : CIR_Op<"switch",
     }
     ```
 
-    The cir.switch might not be considered "simple" if any of the following is
+    The cir.switch is not be considered "simple" if any of the following is
     true:
-    - There are case statements of the switch statement lives in other scopes
+    - There are case statements of the switch statement that are scope
       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,
+    - There is any code before the first case statement. For example,
 
     ```
     switch(int cond) {
@@ -949,7 +948,7 @@ def SwitchOp : CIR_Op<"switch",
   let skipDefaultBuilders = 1;
   let builders = [
     OpBuilder<(ins "mlir::Value":$condition,
-               "llvm::function_ref<void(mlir::OpBuilder &, mlir::Location, mlir::OperationState &)>":$switchBuilder)>
+               "BuilderOpStateCallbackRef":$switchBuilder)>
   ];
 
   let assemblyFormat = [{
@@ -961,12 +960,12 @@ def SwitchOp : CIR_Op<"switch",
 
   let extraClassDeclaration = [{
     // Collect cases in the switch.
-    void collectCases(llvm::SmallVector<CaseOp> &cases);
+    void collectCases(llvm::SmallVectorImpl<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);
+    bool isSimpleForm(llvm::SmallVectorImpl<CaseOp> &cases);
   }];
 }
 

clang/include/clang/CIR/MissingFeatures.h

@@ -162,6 +162,7 @@ struct MissingFeatures {
   static bool moduleNameHash() { return false; }
   static bool setDSOLocal() { return false; }
   static bool foldCaseStmt() { return false; }
+  static bool constantFoldSwitchStatement() { return false; }
 
   // Missing types
   static bool dataMemberType() { return false; }

clang/lib/CIR/CodeGen/CIRGenStmt.cpp

@@ -458,8 +458,9 @@ CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
     } else if (isa<CaseStmt>(sub) && isa<DefaultStmt>(stmt)) {
       subStmtKind = SubStmtKind::Case;
       builder.createYield(loc);
-    } else
+    } else {
       result = emitStmt(sub, /*useCurrentScope=*/!isa<CompoundStmt>(sub));
+    }
 
     insertPoint = builder.saveInsertionPoint();
   }
@@ -496,16 +497,17 @@ CIRGenFunction::emitCaseDefaultCascade(const T *stmt, mlir::Type condType,
   //
   // 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)
+  if (subStmtKind == SubStmtKind::Case) {
     result = emitCaseStmt(*cast<CaseStmt>(sub), condType, buildingTopLevelCase);
-  else if (subStmtKind == SubStmtKind::Default) {
+  } else if (subStmtKind == SubStmtKind::Default) {
     getCIRGenModule().errorNYI(sub->getSourceRange(), "Default case");
     return mlir::failure();
-  } else if (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
     // case to make generating `cir.switch` operation to be a simple form.
     builder.restoreInsertionPoint(insertPoint);
+  }
 
   return result;
 }
@@ -764,6 +766,7 @@ mlir::LogicalResult CIRGenFunction::emitSwitchStmt(const clang::SwitchStmt &s) {
   // only emit live cases. CIR should use MLIR to achieve similar things,
   // nothing to be done here.
   // if (ConstantFoldsToSimpleInteger(S.getCond(), ConstantCondValue))...
+  assert(!cir::MissingFeatures::constantFoldSwitchStatement());
 
   SwitchOp swop;
   auto switchStmtBuilder = [&]() -> mlir::LogicalResult {

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

@@ -882,9 +882,8 @@ void cir::SwitchOp::getSuccessorRegions(
   region.push_back(RegionSuccessor(&getBody()));
 }
 
-void cir::SwitchOp::build(
-    OpBuilder &builder, OperationState &result, Value cond,
-    function_ref<void(OpBuilder &, Location, OperationState &)> switchBuilder) {
+void cir::SwitchOp::build(OpBuilder &builder, OperationState &result,
+                          Value cond, BuilderOpStateCallbackRef switchBuilder) {
   assert(switchBuilder && "the builder callback for regions must be present");
   OpBuilder::InsertionGuard guardSwitch(builder);
   Region *switchRegion = result.addRegion();
@@ -893,7 +892,7 @@ void cir::SwitchOp::build(
   switchBuilder(builder, result.location, result);
 }
 
-void cir::SwitchOp::collectCases(llvm::SmallVector<CaseOp> &cases) {
+void cir::SwitchOp::collectCases(llvm::SmallVectorImpl<CaseOp> &cases) {
   walk<mlir::WalkOrder::PreOrder>([&](mlir::Operation *op) {
     // Don't walk in nested switch op.
     if (isa<cir::SwitchOp>(op) && op != *this)
@@ -906,7 +905,7 @@ void cir::SwitchOp::collectCases(llvm::SmallVector<CaseOp> &cases) {
   });
 }
 
-bool cir::SwitchOp::isSimpleForm(llvm::SmallVector<CaseOp> &cases) {
+bool cir::SwitchOp::isSimpleForm(llvm::SmallVectorImpl<CaseOp> &cases) {
   collectCases(cases);
 
   if (getBody().empty())