Apply code review suggestions and add tests

Andres-Salamanca · Andres-Salamanca · commit eba492424eff · 2025-04-08T16:05:32.000-05:00
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -18,6 +18,7 @@
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
 #include "clang/CIR/MissingFeatures.h"
 
@@ -479,18 +480,10 @@ void CIRGenFunction::emitIgnoredExpr(const Expr *e) {
 mlir::LogicalResult CIRGenFunction::emitIfOnBoolExpr(const Expr *cond,
                                                      const Stmt *thenS,
                                                      const Stmt *elseS) {
-  // Attempt to be more accurate as possible with IfOp location, generate
-  // one fused location that has either 2 or 4 total locations, depending
-  // on else's availability.
-  auto getStmtLoc = [this](const Stmt &s) {
-    return mlir::FusedLoc::get(&getMLIRContext(),
-                               {getLoc(s.getSourceRange().getBegin()),
-                                getLoc(s.getSourceRange().getEnd())});
-  };
-  mlir::Location thenLoc = getStmtLoc(*thenS);
+  mlir::Location thenLoc = getLoc(thenS->getSourceRange());
   std::optional<mlir::Location> elseLoc;
   if (elseS)
-    elseLoc = getStmtLoc(*elseS);
+    elseLoc = getLoc(elseS->getSourceRange());
 
   mlir::LogicalResult resThen = mlir::success(), resElse = mlir::success();
   emitIfOnBoolExpr(
@@ -518,7 +511,9 @@ cir::IfOp CIRGenFunction::emitIfOnBoolExpr(
     const clang::Expr *cond, BuilderCallbackRef thenBuilder,
     mlir::Location thenLoc, BuilderCallbackRef elseBuilder,
     std::optional<mlir::Location> elseLoc) {
-
+  // Attempt to be as accurate as possible with IfOp location, generate
+  // one fused location that has either 2 or 4 total locations, depending
+  // on else's availability.
   SmallVector<mlir::Location, 2> ifLocs{thenLoc};
   if (elseLoc)
     ifLocs.push_back(*elseLoc);
@@ -531,44 +526,34 @@ cir::IfOp CIRGenFunction::emitIfOnBoolExpr(
                                    /*elseBuilder=*/elseBuilder);
 }
 
-/// TODO(cir): PGO data
 /// TODO(cir): see EmitBranchOnBoolExpr for extra ideas).
 mlir::Value CIRGenFunction::emitOpOnBoolExpr(mlir::Location loc,
                                              const Expr *cond) {
-  // TODO(CIR): scoped ApplyDebugLocation DL(*this, Cond);
-  // TODO(CIR): __builtin_unpredictable and profile counts?
+  assert(!cir::MissingFeatures::pgoUse());
+  assert(!cir::MissingFeatures::generateDebugInfo());
   cond = cond->IgnoreParens();
 
-  // if (const BinaryOperator *CondBOp = dyn_cast<BinaryOperator>(cond)) {
-  //   llvm_unreachable("binaryoperator ifstmt NYI");
-  // }
-
-  if (const UnaryOperator *CondUOp = dyn_cast<UnaryOperator>(cond)) {
-    // In LLVM the condition is reversed here for efficient codegen.
-    // This should be done in CIR prior to LLVM lowering, if we do now
-    // we can make CIR based diagnostics misleading.
-    //  cir.ternary(!x, t, f) -> cir.ternary(x, f, t)
-    assert(!cir::MissingFeatures::shouldReverseUnaryCondOnBoolExpr());
-  }
+  // In LLVM the condition is reversed here for efficient codegen.
+  // This should be done in CIR prior to LLVM lowering, if we do now
+  // we can make CIR based diagnostics misleading.
+  //  cir.ternary(!x, t, f) -> cir.ternary(x, f, t)
+  assert(!cir::MissingFeatures::shouldReverseUnaryCondOnBoolExpr());
 
-  if (const ConditionalOperator *CondOp = dyn_cast<ConditionalOperator>(cond)) {
+  if (isa<ConditionalOperator>(cond)) {
 
     cgm.errorNYI(cond->getExprLoc(), "Ternary NYI");
     assert(!cir::MissingFeatures::ternaryOp());
     return createDummyValue(loc, cond->getType());
   }
 
-  // if (const CXXThrowExpr *Throw = dyn_cast<CXXThrowExpr>(cond)) {
-  //   llvm_unreachable("NYI");
-  // }
+  if (isa<CXXThrowExpr>(cond)) {
+    cgm.errorNYI("NYI");
+  }
 
   // If the branch has a condition wrapped by __builtin_unpredictable,
   // create metadata that specifies that the branch is unpredictable.
   // Don't bother if not optimizing because that metadata would not be used.
-  auto *Call = dyn_cast<CallExpr>(cond->IgnoreImpCasts());
-  if (Call && cgm.getCodeGenOpts().OptimizationLevel != 0) {
-    assert(!cir::MissingFeatures::insertBuiltinUnpredictable());
-  }
+  assert(!cir::MissingFeatures::insertBuiltinUnpredictable());
 
   // Emit the code with the fully general case.
   return evaluateExprAsBool(cond);
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -1487,14 +1487,14 @@ mlir::Value ScalarExprEmitter::VisitUnaryLNot(const UnaryOperator *e) {
 /// If the specified expression does not fold
 /// to a constant, or if it does but contains a label, return false.  If it
 /// constant folds return true and set the boolean result in Result.
-bool CIRGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,
-                                                  bool &ResultBool,
-                                                  bool AllowLabels) {
-  llvm::APSInt ResultInt;
-  if (!ConstantFoldsToSimpleInteger(Cond, ResultInt, AllowLabels))
+bool CIRGenFunction::constantFoldsToSimpleInteger(const Expr *cond,
+                                                  bool &resultBool,
+                                                  bool allowLabels) {
+  llvm::APSInt resultInt;
+  if (!constantFoldsToSimpleInteger(cond, resultInt, allowLabels))
     return false;
 
-  ResultBool = ResultInt.getBoolValue();
+  resultBool = resultInt.getBoolValue();
   return true;
 }
 
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -135,7 +135,7 @@ mlir::Location CIRGenFunction::getLoc(mlir::Location lhs, mlir::Location rhs) {
   return mlir::FusedLoc::get(locs, metadata, &getMLIRContext());
 }
 
-bool CIRGenFunction::ContainsLabel(const Stmt *s, bool ignoreCaseStmts) {
+bool CIRGenFunction::containsLabel(const Stmt *s, bool ignoreCaseStmts) {
   // Null statement, not a label!
   if (!s)
     return false;
@@ -160,14 +160,14 @@ bool CIRGenFunction::ContainsLabel(const Stmt *s, bool ignoreCaseStmts) {
   // Scan subexpressions for verboten labels.
   return std::any_of(s->child_begin(), s->child_end(),
                      [=](const Stmt *subStmt) {
-                       return ContainsLabel(subStmt, ignoreCaseStmts);
+                       return containsLabel(subStmt, ignoreCaseStmts);
                      });
 }
 
 /// If the specified expression does not fold
 /// to a constant, or if it does but contains a label, return false.  If it
 /// constant folds return true and set the folded value.
-bool CIRGenFunction::ConstantFoldsToSimpleInteger(const Expr *cond,
+bool CIRGenFunction::constantFoldsToSimpleInteger(const Expr *cond,
                                                   llvm::APSInt &resultInt,
                                                   bool allowLabels) {
   // FIXME: Rename and handle conversion of other evaluatable things
@@ -177,7 +177,7 @@ bool CIRGenFunction::ConstantFoldsToSimpleInteger(const Expr *cond,
     return false; // Not foldable, not integer or not fully evaluatable.
 
   llvm::APSInt intValue = result.Val.getInt();
-  if (!allowLabels && ContainsLabel(cond))
+  if (!allowLabels && containsLabel(cond))
     return false; // Contains a label.
 
   resultInt = intValue;
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -170,16 +170,16 @@ class CIRGenFunction : public CIRGenTypeCache {
   /// If the specified expression does not fold to a constant, or if it does but
   /// contains a label, return false.  If it constant folds return true and set
   /// the boolean result in Result.
-  bool ConstantFoldsToSimpleInteger(const clang::Expr *Cond, bool &ResultBool,
-                                    bool AllowLabels = false);
-  bool ConstantFoldsToSimpleInteger(const clang::Expr *Cond,
-                                    llvm::APSInt &ResultInt,
-                                    bool AllowLabels = false);
+  bool constantFoldsToSimpleInteger(const clang::Expr *cond, bool &resultBool,
+                                    bool allowLabels = false);
+  bool constantFoldsToSimpleInteger(const clang::Expr *cond,
+                                    llvm::APSInt &resultInt,
+                                    bool allowLabels = false);
 
   /// Return true if the statement contains a label in it.  If
   /// this statement is not executed normally, it not containing a label means
   /// that we can just remove the code.
-  bool ContainsLabel(const clang::Stmt *s, bool IgnoreCaseStmts = false);
+  bool containsLabel(const clang::Stmt *s, bool ignoreCaseStmts = false);
 
   struct AutoVarEmission {
     const clang::VarDecl *Variable;
@@ -474,12 +474,12 @@ class CIRGenFunction : public CIRGenTypeCache {
   mlir::LogicalResult emitDeclStmt(const clang::DeclStmt &s);
   LValue emitDeclRefLValue(const clang::DeclRefExpr *e);
 
-  /// Emit an if on a boolean condition to the specified blocks.
+  /// 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. TrueCount should be the number of
-  /// times we expect the condition to evaluate to true based on PGO data. We
-  /// might decide to leave this as a separate pass (see EmitBranchOnBoolExpr
-  /// for extra ideas).
+  /// the conditional based on the branch.
+  /// In the future, we may apply code generation simplifications here,
+  /// similar to those used in classic LLVM  codegen
+  /// See `EmitBranchOnBoolExpr` for inspiration.
   mlir::LogicalResult emitIfOnBoolExpr(const clang::Expr *cond,
                                        const clang::Stmt *thenS,
                                        const clang::Stmt *elseS);
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -286,10 +286,10 @@ mlir::LogicalResult CIRGenFunction::emitIfStmt(const IfStmt &s) {
   mlir::LogicalResult res = mlir::success();
   // The else branch of a consteval if statement is always the only branch
   // that can be runtime evaluated.
-  const Stmt *ConstevalExecuted;
+  const Stmt *constevalExecuted;
   if (s.isConsteval()) {
-    ConstevalExecuted = s.isNegatedConsteval() ? s.getThen() : s.getElse();
-    if (!ConstevalExecuted) {
+    constevalExecuted = s.isNegatedConsteval() ? s.getThen() : s.getElse();
+    if (!constevalExecuted) {
       // No runtime code execution required
       return res;
     }
@@ -299,7 +299,7 @@ mlir::LogicalResult CIRGenFunction::emitIfStmt(const IfStmt &s) {
   // compares unequal to 0.  The condition must be a scalar type.
   auto ifStmtBuilder = [&]() -> mlir::LogicalResult {
     if (s.isConsteval())
-      return emitStmt(ConstevalExecuted, /*useCurrentScope=*/true);
+      return emitStmt(constevalExecuted, /*useCurrentScope=*/true);
 
     if (s.getInit())
       if (emitStmt(s.getInit(), /*useCurrentScope=*/true).failed())
@@ -308,25 +308,22 @@ mlir::LogicalResult CIRGenFunction::emitIfStmt(const IfStmt &s) {
     if (s.getConditionVariable())
       emitDecl(*s.getConditionVariable());
 
-    // During LLVM codegen, if the condition constant folds and can be elided,
-    // it tries to avoid emitting the condition and the dead arm of the if/else.
-    // TODO(cir): we skip this in CIRGen, but should implement this as part of
-    // SSCP or a specific CIR pass.
-    bool CondConstant;
-    if (ConstantFoldsToSimpleInteger(s.getCond(), CondConstant,
+    // If the condition folds to a constant and this is an 'if constexpr',
+    // we simplify it early in CIRGen to avoid emitting the full 'if'.
+    bool condConstant;
+    if (constantFoldsToSimpleInteger(s.getCond(), condConstant,
                                      s.isConstexpr())) {
       if (s.isConstexpr()) {
         // Handle "if constexpr" explicitly here to avoid generating some
         // ill-formed code since in CIR the "if" is no longer simplified
         // in this lambda like in Clang but postponed to other MLIR
         // passes.
-        if (const Stmt *Executed = CondConstant ? s.getThen() : s.getElse())
-          return emitStmt(Executed, /*useCurrentScope=*/true);
+        if (const Stmt *executed = condConstant ? s.getThen() : s.getElse())
+          return emitStmt(executed, /*useCurrentScope=*/true);
         // There is nothing to execute at runtime.
         // TODO(cir): there is still an empty cir.scope generated by the caller.
         return mlir::success();
       }
-      assert(!cir::MissingFeatures::constantFoldsToSimpleInteger());
     }
 
     assert(!cir::MissingFeatures::emitCondLikelihoodViaExpectIntrinsic());
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -520,7 +520,6 @@ ParseResult cir::IfOp::parse(OpAsmParser &parser, OperationState &result) {
 }
 
 void cir::IfOp::print(OpAsmPrinter &p) {
-
   p << " " << getCondition() << " ";
   mlir::Region &thenRegion = this->getThenRegion();
   p.printRegion(thenRegion,
@@ -563,17 +562,6 @@ void cir::IfOp::getSuccessorRegions(mlir::RegionBranchPoint point,
   if (elseRegion->empty())
     elseRegion = nullptr;
 
-  // Otherwise, the successor is dependent on the condition.
-  // bool condition;
-  // if (auto condAttr = operands.front().dyn_cast_or_null<IntegerAttr>()) {
-  //   assert(0 && "not implemented");
-  // condition = condAttr.getValue().isOneValue();
-  // Add the successor regions using the condition.
-  // regions.push_back(RegionSuccessor(condition ? &thenRegion() :
-  // elseRegion));
-  // return;
-  // }
-
   // If the condition isn't constant, both regions may be executed.
   regions.push_back(RegionSuccessor(&getThenRegion()));
   // If the else region does not exist, it is not a viable successor.
diff --git a/clang/test/CIR/CodeGen/if-consteval.cpp b/clang/test/CIR/CodeGen/if-consteval.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
+// RUN: FileCheck --input-file=%t.cir %s --check-prefix=CIR
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t-cir.ll
+// RUN: FileCheck --input-file=%t-cir.ll %s --check-prefix=LLVM
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll
+// RUN: FileCheck --input-file=%t.ll %s --check-prefix=OGCG
+
+void f() {
+  int result = 0;
+
+  if consteval {
+    result = 10; 
+    // CIR-NOT: cir.const #cir.int<10> : !s32i
+    // LLVM-NOT: store i32 10, ptr %1, align 4
+    // OGCG-NOT: store i32 10, ptr %result, align 4
+  } else {
+    result = 20; 
+    // CHECK: cir.const #cir.int<20> : !s32i
+    // LLVM: store i32 20, ptr %1, align 4
+    // OGCG: store i32 20, ptr %result, align 4
+
+  }
+
+  if !consteval {
+    result = 30; 
+    // CIR: cir.const #cir.int<30> : !s32i
+    // LLVM: store i32 30, ptr %1, align 4
+    // OGCG: store i32 30, ptr %result, align 4
+  } else {
+    result = 40; 
+    // CIR-NOT: cir.const #cir.int<40> : !s32i
+    // LLVM-NOT: store i32 40, ptr %1, align 4
+    // OGCG-NOT: store i32 40, ptr %result, align 4
+  }
+
+  if consteval {
+    result = 50; 
+    // CIR-NOT: cir.const #cir.int<50> : !s32i
+    // LLVM-NOT: store i32 50, ptr %1, align 4
+    // OGCG-NOT: store i32 50, ptr %result, align 4
+  }
+
+  if !consteval {
+    result = 60; 
+    // CIR: cir.const #cir.int<60> : !s32i
+    // LLVM: store i32 60, ptr %1, align 4
+    // OGCG: store i32 60, ptr %result, align 4
+  }
+
+}
diff --git a/clang/test/CIR/CodeGen/if.cpp b/clang/test/CIR/CodeGen/if.cpp
