[CIR] Add if statement support #134333
[CIR] Add if statement support #134333
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clangir Author: None (Andres-Salamanca) ChangesThis patch adds support for if statements in the CIR dialect Patch is 43.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134333.diff 13 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
index 4d7f537418a90..4d7f0bfd1c253 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
+++ b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
@@ -35,6 +35,10 @@
using BuilderCallbackRef =
llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>;
+namespace cir {
+void buildTerminatedBody(mlir::OpBuilder &builder, mlir::Location loc);
+} // namespace cir
+
// TableGen'erated files for MLIR dialects require that a macro be defined when
// they are included. GET_OP_CLASSES tells the file to define the classes for
// the operations of that dialect.
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 3965372755685..e181a5db3e1b9 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -424,8 +424,8 @@ def StoreOp : CIR_Op<"store", [
// ReturnOp
//===----------------------------------------------------------------------===//
-def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "DoWhileOp",
- "WhileOp", "ForOp"]>,
+def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "IfOp",
+ "DoWhileOp", "WhileOp", "ForOp"]>,
Terminator]> {
let summary = "Return from function";
let description = [{
@@ -462,6 +462,58 @@ def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "DoWhileOp",
let hasVerifier = 1;
}
+//===----------------------------------------------------------------------===//
+// IfOp
+//===----------------------------------------------------------------------===//
+
+def IfOp : CIR_Op<"if",
+ [DeclareOpInterfaceMethods<RegionBranchOpInterface>,
+ RecursivelySpeculatable, AutomaticAllocationScope, NoRegionArguments]>{
+
+ let summary = "the if-then-else operation";
+ let description = [{
+ The `cir.if` operation represents an if-then-else construct for
+ conditionally executing two regions of code. The operand is a `cir.bool`
+ type.
+
+ Examples:
+
+ ```mlir
+ cir.if %b {
+ ...
+ } else {
+ ...
+ }
+
+ cir.if %c {
+ ...
+ }
+
+ cir.if %c {
+ ...
+ cir.br ^a
+ ^a:
+ cir.yield
+ }
+ ```
+
+ `cir.if` defines no values and the 'else' can be omitted. The if/else
+ regions must be terminated. If the region has only one block, the terminator
+ can be left out, and `cir.yield` terminator will be inserted implictly.
+ Otherwise, the region must be explicitly terminated.
+ }];
+ let arguments = (ins CIR_BoolType:$condition);
+ let regions = (region AnyRegion:$thenRegion, AnyRegion:$elseRegion);
+ let hasCustomAssemblyFormat=1;
+ let hasVerifier=1;
+ let skipDefaultBuilders=1;
+ let builders = [
+ OpBuilder<(ins "mlir::Value":$cond, "bool":$withElseRegion,
+ CArg<"BuilderCallbackRef", "buildTerminatedBody">:$thenBuilder,
+ CArg<"BuilderCallbackRef", "nullptr">:$elseBuilder)>
+ ];
+}
+
//===----------------------------------------------------------------------===//
// ConditionOp
//===----------------------------------------------------------------------===//
@@ -512,8 +564,8 @@ def ConditionOp : CIR_Op<"condition", [
//===----------------------------------------------------------------------===//
def YieldOp : CIR_Op<"yield", [ReturnLike, Terminator,
- ParentOneOf<["ScopeOp", "WhileOp", "ForOp",
- "DoWhileOp"]>]> {
+ ParentOneOf<["IfOp", "ScopeOp", "WhileOp",
+ "ForOp", "DoWhileOp"]>]> {
let summary = "Represents the default branching behaviour of a region";
let description = [{
The `cir.yield` operation terminates regions on different CIR operations,
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 3a102d90aba8f..1d53d094fa4e7 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -81,6 +81,7 @@ struct MissingFeatures {
// Clang early optimizations or things defered to LLVM lowering.
static bool mayHaveIntegerOverflow() { return false; }
+ static bool shouldReverseUnaryCondOnBoolExpr() { return false; }
// Misc
static bool cxxABI() { return false; }
@@ -109,6 +110,9 @@ struct MissingFeatures {
static bool cgFPOptionsRAII() { return false; }
static bool metaDataNode() { return false; }
static bool fastMathFlags() { return false; }
+ static bool constantFoldsToSimpleInteger() { return false; }
+ static bool incrementProfileCounter() { return false; }
+ static bool insertBuiltinUnpredictable() { return false; }
// Missing types
static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
index f01e03a89981d..a12ec878e3656 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -316,6 +316,106 @@ void CIRGenFunction::emitIgnoredExpr(const Expr *e) {
emitLValue(e);
}
+/// Emit an `if` on a boolean condition, filling `then` and `else` into
+/// appropriated regions.
+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);
+ std::optional<mlir::Location> elseLoc;
+ if (elseS)
+ elseLoc = getStmtLoc(*elseS);
+
+ mlir::LogicalResult resThen = mlir::success(), resElse = mlir::success();
+ emitIfOnBoolExpr(
+ cond, /*thenBuilder=*/
+ [&](mlir::OpBuilder &, mlir::Location) {
+ LexicalScope lexScope{*this, thenLoc, builder.getInsertionBlock()};
+ resThen = emitStmt(thenS, /*useCurrentScope=*/true);
+ },
+ thenLoc,
+ /*elseBuilder=*/
+ [&](mlir::OpBuilder &, mlir::Location) {
+ assert(elseLoc && "Invalid location for elseS.");
+ LexicalScope lexScope{*this, *elseLoc, builder.getInsertionBlock()};
+ resElse = emitStmt(elseS, /*useCurrentScope=*/true);
+ },
+ elseLoc);
+
+ return mlir::LogicalResult::success(resThen.succeeded() &&
+ resElse.succeeded());
+}
+
+/// Emit an `if` on a boolean condition, filling `then` and `else` into
+/// appropriated regions.
+cir::IfOp CIRGenFunction::emitIfOnBoolExpr(
+ const clang::Expr *cond, BuilderCallbackRef thenBuilder,
+ mlir::Location thenLoc, BuilderCallbackRef elseBuilder,
+ std::optional<mlir::Location> elseLoc) {
+
+ SmallVector<mlir::Location, 2> ifLocs{thenLoc};
+ if (elseLoc)
+ ifLocs.push_back(*elseLoc);
+ mlir::Location loc = mlir::FusedLoc::get(&getMLIRContext(), ifLocs);
+
+ // Emit the code with the fully general case.
+ mlir::Value condV = emitOpOnBoolExpr(loc, cond);
+ return builder.create<cir::IfOp>(loc, condV, elseLoc.has_value(),
+ /*thenBuilder=*/thenBuilder,
+ /*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?
+ 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());
+ }
+
+ if (const ConditionalOperator *CondOp = dyn_cast<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 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());
+ }
+
+ // Emit the code with the fully general case.
+ return evaluateExprAsBool(cond);
+}
+
mlir::Value CIRGenFunction::emitAlloca(StringRef name, mlir::Type ty,
mlir::Location loc, CharUnits alignment,
bool insertIntoFnEntryBlock,
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
index 2cf92dfbf3a5b..5d85bc6267e8e 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -1358,6 +1358,20 @@ mlir::Value CIRGenFunction::emitScalarConversion(mlir::Value src,
.emitScalarConversion(src, srcTy, dstTy, loc);
}
+/// 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))
+ return false;
+
+ ResultBool = ResultInt.getBoolValue();
+ return true;
+}
+
/// Return the size or alignment of the type of argument of the sizeof
/// expression as an integer.
mlir::Value ScalarExprEmitter::VisitUnaryExprOrTypeTraitExpr(
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 47fc90836fca6..6510ce7985ead 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -135,6 +135,55 @@ 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) {
+ // Null statement, not a label!
+ if (!s)
+ return false;
+
+ // If this is a label, we have to emit the code, consider something like:
+ // if (0) { ... foo: bar(); } goto foo;
+ //
+ // TODO: If anyone cared, we could track __label__'s, since we know that you
+ // can't jump to one from outside their declared region.
+ if (isa<LabelStmt>(s))
+ return true;
+
+ // If this is a case/default statement, and we haven't seen a switch, we
+ // have to emit the code.
+ if (isa<SwitchCase>(s) && !ignoreCaseStmts)
+ return true;
+
+ // If this is a switch statement, we want to ignore cases below it.
+ if (isa<SwitchStmt>(s))
+ ignoreCaseStmts = true;
+
+ // Scan subexpressions for verboten labels.
+ return std::any_of(s->child_begin(), s->child_end(),
+ [=](const Stmt *subStmt) {
+ 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,
+ llvm::APSInt &resultInt,
+ bool allowLabels) {
+ // FIXME: Rename and handle conversion of other evaluatable things
+ // to bool.
+ Expr::EvalResult result;
+ if (!cond->EvaluateAsInt(result, getContext()))
+ return false; // Not foldable, not integer or not fully evaluatable.
+
+ llvm::APSInt intValue = result.Val.getInt();
+ if (!allowLabels && ContainsLabel(cond))
+ return false; // Contains a label.
+
+ resultInt = intValue;
+ return true;
+}
+
void CIRGenFunction::emitAndUpdateRetAlloca(QualType type, mlir::Location loc,
CharUnits alignment) {
if (!type->isVoidType()) {
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 5cae4d5da9516..15b25d8a81522 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -24,6 +24,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/CharUnits.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
#include "clang/CIR/MissingFeatures.h"
@@ -164,6 +165,20 @@ class CIRGenFunction : public CIRGenTypeCache {
/// that it requires no code to be generated.
bool isTrivialInitializer(const Expr *init);
+ /// 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);
+
+ /// 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);
+
struct AutoVarEmission {
const clang::VarDecl *Variable;
/// The address of the alloca for languages with explicit address space
@@ -442,6 +457,25 @@ 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.
+ /// 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).
+ mlir::LogicalResult emitIfOnBoolExpr(const clang::Expr *cond,
+ const clang::Stmt *thenS,
+ const clang::Stmt *elseS);
+ cir::IfOp emitIfOnBoolExpr(const clang::Expr *cond,
+ BuilderCallbackRef thenBuilder,
+ mlir::Location thenLoc,
+ BuilderCallbackRef elseBuilder,
+ std::optional<mlir::Location> elseLoc = {});
+
+ mlir::Value emitOpOnBoolExpr(mlir::Location loc, const clang::Expr *cond);
+
+ mlir::LogicalResult emitIfStmt(const clang::IfStmt &s);
+
/// Emit code to compute the specified expression,
/// ignoring the result.
void emitIgnoredExpr(const clang::Expr *e);
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index b5c1f0ae2a7ef..00a745b7196a0 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -16,6 +16,7 @@
#include "mlir/IR/Builders.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/CIR/MissingFeatures.h"
using namespace clang;
using namespace clang::CIRGen;
@@ -72,7 +73,8 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
assert(outgoing && "expression emission cleared block!");
return mlir::success();
}
-
+ case Stmt::IfStmtClass:
+ return emitIfStmt(cast<IfStmt>(*s));
case Stmt::ForStmtClass:
return emitForStmt(cast<ForStmt>(*s));
case Stmt::WhileStmtClass:
@@ -99,7 +101,6 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
case Stmt::CaseStmtClass:
case Stmt::SEHLeaveStmtClass:
case Stmt::SYCLKernelCallStmtClass:
- case Stmt::IfStmtClass:
case Stmt::SwitchStmtClass:
case Stmt::CoroutineBodyStmtClass:
case Stmt::CoreturnStmtClass:
@@ -263,6 +264,72 @@ static void terminateBody(CIRGenBuilderTy &builder, mlir::Region &r,
b->erase();
}
+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;
+ if (s.isConsteval()) {
+ ConstevalExecuted = s.isNegatedConsteval() ? s.getThen() : s.getElse();
+ if (!ConstevalExecuted) {
+ // No runtime code execution required
+ return res;
+ }
+ }
+
+ // C99 6.8.4.1: The first substatement is executed if the expression
+ // compares unequal to 0. The condition must be a scalar type.
+ auto ifStmtBuilder = [&]() -> mlir::LogicalResult {
+ if (s.isConsteval())
+ return emitStmt(ConstevalExecuted, /*useCurrentScope=*/true);
+
+ if (s.getInit())
+ if (emitStmt(s.getInit(), /*useCurrentScope=*/true).failed())
+ return mlir::failure();
+
+ 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,
+ 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);
+ // 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());
+ assert(!cir::MissingFeatures::incrementProfileCounter());
+ return emitIfOnBoolExpr(s.getCond(), s.getThen(), s.getElse());
+ };
+
+ // TODO: Add a new scoped symbol table.
+ // LexicalScope ConditionScope(*this, S.getCond()->getSourceRange());
+ // The if scope contains the full source range for IfStmt.
+ mlir::Location scopeLoc = getLoc(s.getSourceRange());
+ builder.create<cir::ScopeOp>(
+ scopeLoc, /*scopeBuilder=*/
+ [&](mlir::OpBuilder &b, mlir::Location loc) {
+ LexicalScope lexScope{*this, scopeLoc, builder.getInsertionBlock()};
+ res = ifStmtBuilder();
+ });
+
+ return res;
+}
+
mlir::LogicalResult CIRGenFunction::emitDeclStmt(const DeclStmt &s) {
assert(builder.getInsertionBlock() && "expected valid insertion point");
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDi...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: None (Andres-Salamanca) ChangesThis patch adds support for if statements in the CIR dialect Patch is 43.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134333.diff 13 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
index 4d7f537418a90..4d7f0bfd1c253 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
+++ b/clang/include/clang/CIR/Dialect/IR/CIRDialect.h
@@ -35,6 +35,10 @@
using BuilderCallbackRef =
llvm::function_ref<void(mlir::OpBuilder &, mlir::Location)>;
+namespace cir {
+void buildTerminatedBody(mlir::OpBuilder &builder, mlir::Location loc);
+} // namespace cir
+
// TableGen'erated files for MLIR dialects require that a macro be defined when
// they are included. GET_OP_CLASSES tells the file to define the classes for
// the operations of that dialect.
diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index 3965372755685..e181a5db3e1b9 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -424,8 +424,8 @@ def StoreOp : CIR_Op<"store", [
// ReturnOp
//===----------------------------------------------------------------------===//
-def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "DoWhileOp",
- "WhileOp", "ForOp"]>,
+def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "IfOp",
+ "DoWhileOp", "WhileOp", "ForOp"]>,
Terminator]> {
let summary = "Return from function";
let description = [{
@@ -462,6 +462,58 @@ def ReturnOp : CIR_Op<"return", [ParentOneOf<["FuncOp", "ScopeOp", "DoWhileOp",
let hasVerifier = 1;
}
+//===----------------------------------------------------------------------===//
+// IfOp
+//===----------------------------------------------------------------------===//
+
+def IfOp : CIR_Op<"if",
+ [DeclareOpInterfaceMethods<RegionBranchOpInterface>,
+ RecursivelySpeculatable, AutomaticAllocationScope, NoRegionArguments]>{
+
+ let summary = "the if-then-else operation";
+ let description = [{
+ The `cir.if` operation represents an if-then-else construct for
+ conditionally executing two regions of code. The operand is a `cir.bool`
+ type.
+
+ Examples:
+
+ ```mlir
+ cir.if %b {
+ ...
+ } else {
+ ...
+ }
+
+ cir.if %c {
+ ...
+ }
+
+ cir.if %c {
+ ...
+ cir.br ^a
+ ^a:
+ cir.yield
+ }
+ ```
+
+ `cir.if` defines no values and the 'else' can be omitted. The if/else
+ regions must be terminated. If the region has only one block, the terminator
+ can be left out, and `cir.yield` terminator will be inserted implictly.
+ Otherwise, the region must be explicitly terminated.
+ }];
+ let arguments = (ins CIR_BoolType:$condition);
+ let regions = (region AnyRegion:$thenRegion, AnyRegion:$elseRegion);
+ let hasCustomAssemblyFormat=1;
+ let hasVerifier=1;
+ let skipDefaultBuilders=1;
+ let builders = [
+ OpBuilder<(ins "mlir::Value":$cond, "bool":$withElseRegion,
+ CArg<"BuilderCallbackRef", "buildTerminatedBody">:$thenBuilder,
+ CArg<"BuilderCallbackRef", "nullptr">:$elseBuilder)>
+ ];
+}
+
//===----------------------------------------------------------------------===//
// ConditionOp
//===----------------------------------------------------------------------===//
@@ -512,8 +564,8 @@ def ConditionOp : CIR_Op<"condition", [
//===----------------------------------------------------------------------===//
def YieldOp : CIR_Op<"yield", [ReturnLike, Terminator,
- ParentOneOf<["ScopeOp", "WhileOp", "ForOp",
- "DoWhileOp"]>]> {
+ ParentOneOf<["IfOp", "ScopeOp", "WhileOp",
+ "ForOp", "DoWhileOp"]>]> {
let summary = "Represents the default branching behaviour of a region";
let description = [{
The `cir.yield` operation terminates regions on different CIR operations,
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 3a102d90aba8f..1d53d094fa4e7 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -81,6 +81,7 @@ struct MissingFeatures {
// Clang early optimizations or things defered to LLVM lowering.
static bool mayHaveIntegerOverflow() { return false; }
+ static bool shouldReverseUnaryCondOnBoolExpr() { return false; }
// Misc
static bool cxxABI() { return false; }
@@ -109,6 +110,9 @@ struct MissingFeatures {
static bool cgFPOptionsRAII() { return false; }
static bool metaDataNode() { return false; }
static bool fastMathFlags() { return false; }
+ static bool constantFoldsToSimpleInteger() { return false; }
+ static bool incrementProfileCounter() { return false; }
+ static bool insertBuiltinUnpredictable() { return false; }
// Missing types
static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
index f01e03a89981d..a12ec878e3656 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -316,6 +316,106 @@ void CIRGenFunction::emitIgnoredExpr(const Expr *e) {
emitLValue(e);
}
+/// Emit an `if` on a boolean condition, filling `then` and `else` into
+/// appropriated regions.
+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);
+ std::optional<mlir::Location> elseLoc;
+ if (elseS)
+ elseLoc = getStmtLoc(*elseS);
+
+ mlir::LogicalResult resThen = mlir::success(), resElse = mlir::success();
+ emitIfOnBoolExpr(
+ cond, /*thenBuilder=*/
+ [&](mlir::OpBuilder &, mlir::Location) {
+ LexicalScope lexScope{*this, thenLoc, builder.getInsertionBlock()};
+ resThen = emitStmt(thenS, /*useCurrentScope=*/true);
+ },
+ thenLoc,
+ /*elseBuilder=*/
+ [&](mlir::OpBuilder &, mlir::Location) {
+ assert(elseLoc && "Invalid location for elseS.");
+ LexicalScope lexScope{*this, *elseLoc, builder.getInsertionBlock()};
+ resElse = emitStmt(elseS, /*useCurrentScope=*/true);
+ },
+ elseLoc);
+
+ return mlir::LogicalResult::success(resThen.succeeded() &&
+ resElse.succeeded());
+}
+
+/// Emit an `if` on a boolean condition, filling `then` and `else` into
+/// appropriated regions.
+cir::IfOp CIRGenFunction::emitIfOnBoolExpr(
+ const clang::Expr *cond, BuilderCallbackRef thenBuilder,
+ mlir::Location thenLoc, BuilderCallbackRef elseBuilder,
+ std::optional<mlir::Location> elseLoc) {
+
+ SmallVector<mlir::Location, 2> ifLocs{thenLoc};
+ if (elseLoc)
+ ifLocs.push_back(*elseLoc);
+ mlir::Location loc = mlir::FusedLoc::get(&getMLIRContext(), ifLocs);
+
+ // Emit the code with the fully general case.
+ mlir::Value condV = emitOpOnBoolExpr(loc, cond);
+ return builder.create<cir::IfOp>(loc, condV, elseLoc.has_value(),
+ /*thenBuilder=*/thenBuilder,
+ /*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?
+ 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());
+ }
+
+ if (const ConditionalOperator *CondOp = dyn_cast<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 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());
+ }
+
+ // Emit the code with the fully general case.
+ return evaluateExprAsBool(cond);
+}
+
mlir::Value CIRGenFunction::emitAlloca(StringRef name, mlir::Type ty,
mlir::Location loc, CharUnits alignment,
bool insertIntoFnEntryBlock,
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
index 2cf92dfbf3a5b..5d85bc6267e8e 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -1358,6 +1358,20 @@ mlir::Value CIRGenFunction::emitScalarConversion(mlir::Value src,
.emitScalarConversion(src, srcTy, dstTy, loc);
}
+/// 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))
+ return false;
+
+ ResultBool = ResultInt.getBoolValue();
+ return true;
+}
+
/// Return the size or alignment of the type of argument of the sizeof
/// expression as an integer.
mlir::Value ScalarExprEmitter::VisitUnaryExprOrTypeTraitExpr(
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 47fc90836fca6..6510ce7985ead 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -135,6 +135,55 @@ 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) {
+ // Null statement, not a label!
+ if (!s)
+ return false;
+
+ // If this is a label, we have to emit the code, consider something like:
+ // if (0) { ... foo: bar(); } goto foo;
+ //
+ // TODO: If anyone cared, we could track __label__'s, since we know that you
+ // can't jump to one from outside their declared region.
+ if (isa<LabelStmt>(s))
+ return true;
+
+ // If this is a case/default statement, and we haven't seen a switch, we
+ // have to emit the code.
+ if (isa<SwitchCase>(s) && !ignoreCaseStmts)
+ return true;
+
+ // If this is a switch statement, we want to ignore cases below it.
+ if (isa<SwitchStmt>(s))
+ ignoreCaseStmts = true;
+
+ // Scan subexpressions for verboten labels.
+ return std::any_of(s->child_begin(), s->child_end(),
+ [=](const Stmt *subStmt) {
+ 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,
+ llvm::APSInt &resultInt,
+ bool allowLabels) {
+ // FIXME: Rename and handle conversion of other evaluatable things
+ // to bool.
+ Expr::EvalResult result;
+ if (!cond->EvaluateAsInt(result, getContext()))
+ return false; // Not foldable, not integer or not fully evaluatable.
+
+ llvm::APSInt intValue = result.Val.getInt();
+ if (!allowLabels && ContainsLabel(cond))
+ return false; // Contains a label.
+
+ resultInt = intValue;
+ return true;
+}
+
void CIRGenFunction::emitAndUpdateRetAlloca(QualType type, mlir::Location loc,
CharUnits alignment) {
if (!type->isVoidType()) {
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 5cae4d5da9516..15b25d8a81522 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -24,6 +24,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/CharUnits.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
#include "clang/CIR/MissingFeatures.h"
@@ -164,6 +165,20 @@ class CIRGenFunction : public CIRGenTypeCache {
/// that it requires no code to be generated.
bool isTrivialInitializer(const Expr *init);
+ /// 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);
+
+ /// 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);
+
struct AutoVarEmission {
const clang::VarDecl *Variable;
/// The address of the alloca for languages with explicit address space
@@ -442,6 +457,25 @@ 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.
+ /// 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).
+ mlir::LogicalResult emitIfOnBoolExpr(const clang::Expr *cond,
+ const clang::Stmt *thenS,
+ const clang::Stmt *elseS);
+ cir::IfOp emitIfOnBoolExpr(const clang::Expr *cond,
+ BuilderCallbackRef thenBuilder,
+ mlir::Location thenLoc,
+ BuilderCallbackRef elseBuilder,
+ std::optional<mlir::Location> elseLoc = {});
+
+ mlir::Value emitOpOnBoolExpr(mlir::Location loc, const clang::Expr *cond);
+
+ mlir::LogicalResult emitIfStmt(const clang::IfStmt &s);
+
/// Emit code to compute the specified expression,
/// ignoring the result.
void emitIgnoredExpr(const clang::Expr *e);
diff --git a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
index b5c1f0ae2a7ef..00a745b7196a0 100644
--- a/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenStmt.cpp
@@ -16,6 +16,7 @@
#include "mlir/IR/Builders.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/CIR/MissingFeatures.h"
using namespace clang;
using namespace clang::CIRGen;
@@ -72,7 +73,8 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
assert(outgoing && "expression emission cleared block!");
return mlir::success();
}
-
+ case Stmt::IfStmtClass:
+ return emitIfStmt(cast<IfStmt>(*s));
case Stmt::ForStmtClass:
return emitForStmt(cast<ForStmt>(*s));
case Stmt::WhileStmtClass:
@@ -99,7 +101,6 @@ mlir::LogicalResult CIRGenFunction::emitStmt(const Stmt *s,
case Stmt::CaseStmtClass:
case Stmt::SEHLeaveStmtClass:
case Stmt::SYCLKernelCallStmtClass:
- case Stmt::IfStmtClass:
case Stmt::SwitchStmtClass:
case Stmt::CoroutineBodyStmtClass:
case Stmt::CoreturnStmtClass:
@@ -263,6 +264,72 @@ static void terminateBody(CIRGenBuilderTy &builder, mlir::Region &r,
b->erase();
}
+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;
+ if (s.isConsteval()) {
+ ConstevalExecuted = s.isNegatedConsteval() ? s.getThen() : s.getElse();
+ if (!ConstevalExecuted) {
+ // No runtime code execution required
+ return res;
+ }
+ }
+
+ // C99 6.8.4.1: The first substatement is executed if the expression
+ // compares unequal to 0. The condition must be a scalar type.
+ auto ifStmtBuilder = [&]() -> mlir::LogicalResult {
+ if (s.isConsteval())
+ return emitStmt(ConstevalExecuted, /*useCurrentScope=*/true);
+
+ if (s.getInit())
+ if (emitStmt(s.getInit(), /*useCurrentScope=*/true).failed())
+ return mlir::failure();
+
+ 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,
+ 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);
+ // 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());
+ assert(!cir::MissingFeatures::incrementProfileCounter());
+ return emitIfOnBoolExpr(s.getCond(), s.getThen(), s.getElse());
+ };
+
+ // TODO: Add a new scoped symbol table.
+ // LexicalScope ConditionScope(*this, S.getCond()->getSourceRange());
+ // The if scope contains the full source range for IfStmt.
+ mlir::Location scopeLoc = getLoc(s.getSourceRange());
+ builder.create<cir::ScopeOp>(
+ scopeLoc, /*scopeBuilder=*/
+ [&](mlir::OpBuilder &b, mlir::Location loc) {
+ LexicalScope lexScope{*this, scopeLoc, builder.getInsertionBlock()};
+ res = ifStmtBuilder();
+ });
+
+ return res;
+}
+
mlir::LogicalResult CIRGenFunction::emitDeclStmt(const DeclStmt &s) {
assert(builder.getInsertionBlock() && "expected valid insertion point");
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDi...
[truncated]
|
| Examples: | ||
|
|
||
| ```mlir | ||
| cir.if %b { |
There was a problem hiding this comment.
is %b here the condition? Can we make that more clear by renaming this variable?
This differs from the loops, where the condition has its own block. I find myself curious why that is. Also, what happens with variable declarations in the condition? Do we properly set scope for those?
There was a problem hiding this comment.
I'm guessing the reason for the difference is that the condition only needs to be executed once for if statements. Any operations needed to evaluate the condition will appear in the lexical scope enclosing the if operation, similar to how the init section of for loops are handled.
I think it would be good to change for consistency (and maybe also revisit the init section of for loops).
Consider:
if (int x = *p)
n += x;
Gives us:
cir.scope {
%3 = cir.alloca !s32i, !cir.ptr<!s32i>, ["x", init]
%4 = cir.load deref %0 : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i>
%5 = cir.load %4 : !cir.ptr<!s32i>, !s32i
cir.store %5, %3 : !s32i, !cir.ptr<!s32i>
%6 = cir.load %3 : !cir.ptr<!s32i>, !s32i
%7 = cir.cast(int_to_bool, %6 : !s32i), !cir.bool
cir.if %7 {
%8 = cir.load %3 : !cir.ptr<!s32i>, !s32i
%9 = cir.load %1 : !cir.ptr<!s32i>, !s32i
%10 = cir.binop(add, %9, %8) nsw : !s32i
cir.store %10, %1 : !s32i, !cir.ptr<!s32i>
}
}
But
while (int x = *p)
n += x;
Gives us:
cir.scope {
%3 = cir.alloca !s32i, !cir.ptr<!s32i>, ["x", init]
cir.while {
%4 = cir.load deref %0 : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i>
%5 = cir.load %4 : !cir.ptr<!s32i>, !s32i
cir.store %5, %3 : !s32i, !cir.ptr<!s32i>
%6 = cir.load %3 : !cir.ptr<!s32i>, !s32i
%7 = cir.cast(int_to_bool, %6 : !s32i), !cir.bool
cir.condition(%7)
} do {
%4 = cir.load %3 : !cir.ptr<!s32i>, !s32i
%5 = cir.load %1 : !cir.ptr<!s32i>, !s32i
%6 = cir.binop(add, %5, %4) nsw : !s32i
cir.store %6, %1 : !s32i, !cir.ptr<!s32i>
cir.yield
}
}
Maybe the if representation should be:
cir.scope {
%3 = cir.alloca !s32i, !cir.ptr<!s32i>, ["x", init]
cir.if {
%4 = cir.load deref %0 : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i>
%5 = cir.load %4 : !cir.ptr<!s32i>, !s32i
cir.store %5, %3 : !s32i, !cir.ptr<!s32i>
%6 = cir.load %3 : !cir.ptr<!s32i>, !s32i
%7 = cir.cast(int_to_bool, %6 : !s32i), !cir.bool
cir.condition(%7)
} then {
%8 = cir.load %3 : !cir.ptr<!s32i>, !s32i
%9 = cir.load %1 : !cir.ptr<!s32i>, !s32i
%10 = cir.binop(add, %9, %8) nsw : !s32i
cir.store %10, %1 : !s32i, !cir.ptr<!s32i>
}
}
@bcardosolopes What do you think?
There was a problem hiding this comment.
This differs from the loops, where the condition has its own block. I find myself curious why that is.
Like Andy said, if is significantly more simple. Worth mentioning that we redesigned loops 3 or 4 times already, and more will likely come once loop-based CIR transformations get explored.
Also, what happens with variable declarations in the condition? Do we properly set scope for those?
It's in the scope. My long term plan here has been to tag the scopes (e.g. cir.scope if), and all the other variations we see in CIR's LexicalScope right now), should help clarify some of the source code connections and passes might have an easier time when searching for specific parents.
What do you think?
IMO adding one more region and an operation to yield only creates more complications for analysis: operations that were previously dominating are now in a region sibling with then/else - something that could have been a dominator check now will envolve looking at other regions, for no clear benefit I can see at the moment, do you have anything in mind? My experience so far with using regions is that C/C++ often backstabs some nice design attemps, so I'm more on the skeptical side. If you feel encourage to try this approach, I recommend experimenting in the incubator to double check IR soundness with our CIR test coverage.
There was a problem hiding this comment.
do you have anything in mind?
I don't have anything specific in mind. I just have a general preference for consistency. If I've written some code to handle cir.if and it knows how to find the condition and any associated initialization, then I want to extend this code to handle cir.while, it would surprise me to have to find the condition and initialization differently. Of course, without actually going through that process it's hard to say which way is more natural in practice.
If you feel encourage to try this approach, I recommend experimenting in the incubator to double check IR soundness with our CIR test coverage.
I am OK with having this PR proceed with the current representation and revisiting the issue in the incubator if we decide to explore a change. At least until we have enough of the implementation upstreamed to believe that people might be using it, I would agree that's a good general approach.
There was a problem hiding this comment.
FWIW, I feel the above conversation/future directions is enough for me, so discuss/evolve as we go, but feel free to move on with the current representaiton here.
| return true; | ||
|
|
||
| // If this is a switch statement, we want to ignore cases below it. | ||
| if (isa<SwitchStmt>(s)) |
There was a problem hiding this comment.
I don't think this is necessary? SwitchStmt we probably still want the lables for (since you can jump into them), but CaseStmt are the ones we want to ignore.
So the comment isn't really accurate
There was a problem hiding this comment.
This matches OG and it's probably being conservative, I wouldn't go about changing it right now (at least not until this is also tested / changed in OG as well).
There was a problem hiding this comment.
Its actually being the inverse of conservative, isn't it? It is going to end up skipping blocks that it shouldn't, causing compile-issues.
There was a problem hiding this comment.
I dug into this a bit more and I think the code here is correct. This function is calling itself recursively to walk through all the sub-statements in the original statement. If we hit a case statement without having seen a switch statement, we treat the case statement as a label (on line 153 above). However, if we have seen a switch statement and we're recursing into the sub-statements of that switch statement, then case statements are expected and we don't want to treat them as labels.
This is basically happening in the context of determining whether or not we can constant-fold the statement. I put together an experiment to show how the classic codegen handles labels in a switch statement.
https://godbolt.org/z/dY14fWezT
If there is a label in the middle of a switch statement, an if statement that uses that switch (foo()) won't be constant folded, but the same switch without labels (bar()) is folded.
Note that the clang IR incubator doesn't fold either case, but it does if the if is part of a constexpr.
|
|
||
| int if0(bool a) { | ||
|
|
||
| if (a) |
There was a problem hiding this comment.
We should have tests for the init section of an if (if (auto x = something; x.something_else)), or the 'init-as-condition' case (if (auto x = something)), and ALSO that both of those are in scope in the else.
There was a problem hiding this comment.
I’ve added a test for the if (auto x = something) case let me know if it looks good now!
I didn’t include the if (auto x = something; x.something_else) style yet because I was thinking of doing something like:
int if_init_and_cond() {
if (int x = 42; x > 0) {
return x + 2;
} else {
return x - 2;
}
}but x > 0 fails with BinaryOperator Not Yet Implemented:
If if (int x = 42; true) is good enough for now, I can go ahead and add a test for that too just let me know what you prefer.
There was a problem hiding this comment.
Hmm... the idea of having a test BASED on 'x' on the RHS was important.
Do we have int-to-bool decay that you could do if (int x = 42; x) ?
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| mlir::LogicalResult CIRGenFunction::emitIfOnBoolExpr(const Expr *cond, | ||
| const Stmt *thenS, | ||
| const Stmt *elseS) { | ||
| // Attempt to be more accurate as possible with IfOp location, generate |
There was a problem hiding this comment.
| // Attempt to be more accurate as possible with IfOp location, generate | |
| // Attempt to be as accurate as possible with IfOp location, generate |
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| 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. |
There was a problem hiding this comment.
This comment feels misplaced to me. The code to fuse the if locations and the else locations is in the function below. At first glance, not seeing that happening in this function, I thought maybe the comment was out-of-date. Moving the comment to the function where the locations are fused would clarify this.
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| // 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(), |
There was a problem hiding this comment.
I'm pretty sure this is unnecessary. CIRGenFunction::getLoc(SourceRange) does this.
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| {getLoc(s.getSourceRange().getBegin()), | ||
| getLoc(s.getSourceRange().getEnd())}); | ||
| }; | ||
| mlir::Location thenLoc = getStmtLoc(*thenS); |
There was a problem hiding this comment.
| mlir::Location thenLoc = getStmtLoc(*thenS); | |
| mlir::Location thenLoc = getLoc(thenS->getSourceRange()); |
Also, it would be good to assert that thenS isn't null before dereferencing it here.
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| mlir::Location thenLoc = getStmtLoc(*thenS); | ||
| std::optional<mlir::Location> elseLoc; | ||
| if (elseS) | ||
| elseLoc = getStmtLoc(*elseS); |
There was a problem hiding this comment.
| elseLoc = getStmtLoc(*elseS); | |
| elseLoc = getLoc(elseS->getSourceRange()); |
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
|
|
||
| /// 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 |
There was a problem hiding this comment.
I'm not sure what this comment means. What is "TrueCount"? @bcardosolopes?
There was a problem hiding this comment.
This comment comes from OG, where the interface takes PGO information into account
/// EmitBranchOnBoolExpr - Emit a branch on a boolean condition (e.g. for an
/// if statement) to the specified blocks. 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.
void EmitBranchOnBoolExpr(const Expr *Cond, llvm::BasicBlock *TrueBlock,
llvm::BasicBlock *FalseBlock, uint64_t TrueCount,
Stmt::Likelihood LH = Stmt::LH_None,
const Expr *ConditionalOp = nullptr);
There was a problem hiding this comment.
OK, so the comment here is basically saying we should consider performing the simplifications that are performed in the classic codegen. @Andres-Salamanca Can you reword the comment to reflect that? You should removed the sentence about TrueCount.
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
| // that can be runtime evaluated. | ||
| const Stmt *ConstevalExecuted; | ||
| if (s.isConsteval()) { | ||
| ConstevalExecuted = s.isNegatedConsteval() ? s.getThen() : s.getElse(); |
There was a problem hiding this comment.
Please add test cases for the consteval cases supported here.
There was a problem hiding this comment.
I added the test to a new if-consteval.cpp, based on this file. Since we don't support call op yet, I tweaked the test slightly.
Also added the extra RUN: line.
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
| // 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, |
There was a problem hiding this comment.
The only place the classic codegen calls ConstantFoldsToSimpleInteger that isn't using the form with a Boolean result argument is in the switch statement handling. That case is not folded in the CIR codegen in the incubator, with a comment saying we prefer to keep it and let MLIR do the folding. @bcardosolopes Wouldn't the same apply here and other places where conditions are constant folded?
It looks like @keryell added the implementation here and maybe is needed in the case of constexpr.
There was a problem hiding this comment.
Wouldn't the same apply here and other places where conditions are constant folded?
I have gone back-n-forth with the general guideline here, hence the inconsistencies. Doing it in MLIR is probably nicer, but if we can "easily" (not too compile time intensive) do it during CIRGen, shouldn't we? A lot of places that don't do it in OG are probably because people usually first try it out in LLVM IR directly, and there are less clang experts than folks writing LLVM passes - all of that to say that sometimes CodeGen is just missing an opportunity we saw fit when writing CIRGen.
I'm still hesitant to do it anywhere it could affect static analysis quality (doesn't seem like a case we would care here though?)
There was a problem hiding this comment.
Doing it in MLIR is probably nicer, but if we can "easily" (not too compile time intensive) do it during CIRGen, shouldn't we?
I guess this is a philosophical question, but I'm of the opinion that the front end should strictly be producing IR that represents the semantics of the source code and has no business doing any sort of optimization. It took me a while to come to this conclusion when I was working on the optimizer, because it was jarring how inefficient the IR looked coming out of the front end (things like storing a variable to an alloca slot and then immediately reloading it, but I eventually came to realize that the front end was doing that because those were the unvarnished semantics of the source code. Taking that to the extreme, if the source code says "if (true)..." what business does the front end have removing that? Optimizations should be left to the optimizer.
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
| if (s.getConditionVariable()) | ||
| emitDecl(*s.getConditionVariable()); | ||
|
|
||
| // During LLVM codegen, if the condition constant folds and can be elided, |
There was a problem hiding this comment.
This comment predates the implementation below and is now stale. If we keep the folding, this comment should be updated.
There was a problem hiding this comment.
// 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'.
is this better ?
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
clang/lib/CIR/CodeGen/CIRGenStmt.cpp
Outdated
|
|
||
| // 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 |
There was a problem hiding this comment.
I think this part of the comment is just wrong. It assumes we aren't doing the folding here.
There was a problem hiding this comment.
Yea, seems like there's still a assert(!cir::MissingFeatures::constantFoldsToSimpleInteger()); there even though this has been implemented. Will clean that up, thanks!
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
| return mlir::FusedLoc::get(locs, metadata, &getMLIRContext()); | ||
| } | ||
|
|
||
| bool CIRGenFunction::ContainsLabel(const Stmt *s, bool ignoreCaseStmts) { |
There was a problem hiding this comment.
this should be camelBack -> containsLabel, similar for anything else introduced in this PR
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
|
@Andres-Salamanca thanks for working on this. We usually write tests that exercise any possible different paths codegen can take. For next rounds: it's fine if you add less things in one go and make it more incremental with the use of (a) errorNYI and (b) assert on missing features. |
We should probably make it a standard practice when upstreaming new ops like this that if the change goes above 500 lines or so, we should leave the lowering to LLVM IR and related transformations for a follow-up PR. |
| } | ||
|
|
||
| void cir::IfOp::print(OpAsmPrinter &p) { | ||
|
|
There was a problem hiding this comment.
I don't like the blank line at the start of the function. I'm a little surprised clang-format doesn't remove that.
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
|
|
||
| // Otherwise, the successor is dependent on the condition. | ||
| // bool condition; | ||
| // if (auto condAttr = operands.front().dyn_cast_or_null<IntegerAttr>()) { |
There was a problem hiding this comment.
I'm not sure what this code was doing or when it was commented out. It should be removed.
There was a problem hiding this comment.
Changes applied Let me know if it looks good now
| if (ifOp->getResults().empty()) | ||
| continueBlock = remainingOpsBlock; | ||
| else | ||
| llvm_unreachable("NYI"); |
There was a problem hiding this comment.
@bcardosolopes What does this mean? What's not implemented here?
andykaylor
left a comment
andykaylor
left a comment
There was a problem hiding this comment.
I have mostly minor comments. This looks nearly ready.
There was a problem hiding this comment.
Nit: The second line here should be indented one more space to align with the "FuncOp" item above.
There was a problem hiding this comment.
| cir.if %b { | |
| cir.if %cond { |
This still needs to be updated.
There was a problem hiding this comment.
| "ForOp", "DoWhileOp"]>]> { | |
| "ForOp", "DoWhileOp"]>]> { |
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
There was a problem hiding this comment.
The formatting of this comment is off. Please make it wrap in more logical places.
There was a problem hiding this comment.
Braces aren't needed here since the body has a single line.
There was a problem hiding this comment.
| // LLVM-NOT: store i32 10, ptr %1, align 4 | |
| // LLVM-NOT: store i32 10 |
Similar changes should be made for the other "NOT" checks. The "NOT" checks should be as limited as possible to be sure the statement isn't generated. For instance, in this case if the compiler generated store i32 10, ptr %2, align 4 the test would pass because the difference in variable name would cause the line not to be matched.
There was a problem hiding this comment.
🤯🤯 I hadn’t thought of that
sounds good to me |
Upstream if statement support Formatted code and added test cases. added multiple RUN lines for the codegen test
andykaylor
left a comment
andykaylor
left a comment
There was a problem hiding this comment.
I have just a few minor requests. Otherwise, this looks good to me.
| static bool lvalueBaseInfo() { return false; } | ||
| static bool alignCXXRecordDecl() { return false; } | ||
| static bool setNonGC() { return false; } | ||
| static bool constantFoldsToSimpleInteger() { return false; } |
There was a problem hiding this comment.
This isn't used now. It can be removed.
| /// 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, |
There was a problem hiding this comment.
Can you rename this to constantFoldsToBool and move it to the same file as the other constantFoldsToInteger implementation?
| if (isa<SwitchCase>(s) && !ignoreCaseStmts) | ||
| return true; | ||
|
|
||
| // If this is a switch statement, we want to ignore cases below it. |
There was a problem hiding this comment.
| // If this is a switch statement, we want to ignore cases below it. | |
| // If this is a switch statement, we want to ignore case statements when we | |
| // recursively process the sub-statements of the switch. If we haven't | |
| // encountered a switch statement, we treat case statements like labels, but | |
| // if we are processing a switch statement, case statements are expected. |
This is intended to capture the outcome of the discussion below.
andykaylor
left a comment
andykaylor
left a comment
There was a problem hiding this comment.
lgtm
Thanks for working on this!
|
@Andres-Salamanca Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
5 participants
This patch adds support for if statements in the CIR dialect
Additionally, multiple RUN lines were introduced to improve codegen test coverage