Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 7, 2025
+442 −43
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ff564e6
add support for anyOf, default, and range case kinds
Andres-Salamanca Apr 30, 2025
ee0b0aa
Remove foldcase from missing features
Andres-Salamanca Apr 30, 2025
3142924
Fix code formatting
Andres-Salamanca Apr 30, 2025
b032de7
Fix code formatting
Andres-Salamanca Apr 30, 2025
39861dd
Remove early optimization for cascading cases and update test cases a…
Andres-Salamanca May 3, 2025
c772736
Add foldCascadingCases to MissingFeatures and update case handling
Andres-Salamanca May 5, 2025
906e697
remove foldCaseStmt
Andres-Salamanca May 5, 2025
f470023
Updated test with pattern matching and removed caseEltValueListAttr v…
Andres-Salamanca May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/include/clang/CIR/MissingFeatures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ struct MissingFeatures {
// Unary operator handling
static bool opUnaryPromotionType() { return false; }

// SwitchOp handling
static bool foldRangeCase() { return false; }

// Clang early optimizations or things defered to LLVM lowering.
static bool mayHaveIntegerOverflow() { return false; }
static bool shouldReverseUnaryCondOnBoolExpr() { return false; }
Expand Down Expand Up @@ -161,7 +164,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
Expand Down
7 changes: 7 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -532,6 +535,10 @@ 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.
Expand Down
56 changes: 41 additions & 15 deletions clang/lib/CIR/CodeGen/CIRGenStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ mlir::LogicalResult CIRGenFunction::emitSimpleStmt(const Stmt *s,
case Stmt::NullStmtClass:
break;
case Stmt::CaseStmtClass:
case Stmt::DefaultStmtClass:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// If we reached here, we must not handling a switch case in the top level.
return emitSwitchCase(cast<SwitchCase>(*s),
/*buildingTopLevelCase=*/false);
Expand Down Expand Up @@ -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)) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, okay I’ll change it

(isa<CaseStmt>(sub) && isa<CaseStmt>(stmt))) {
subStmtKind = SubStmtKind::Case;
builder.createYield(loc);
} else {
Expand Down Expand Up @@ -500,8 +502,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
Expand All @@ -515,19 +517,44 @@ 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());
cir::CaseOpKind kind;
mlir::ArrayAttr value;

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();
llvm::APSInt intVal = s.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 = s.getRHS()) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment to mention implementing this in CIRSimplify.

} else {
caseEltValueListAttr.push_back(cir::IntAttr::get(condType, intVal));
value = builder.getArrayAttr(caseEltValueListAttr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

kind = cir::CaseOpKind::Equal;
}
assert(!cir::MissingFeatures::foldCaseStmt());
return emitCaseDefaultCascade(&s, condType, value, cir::CaseOpKind::Equal,

return emitCaseDefaultCascade(&s, 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() &&
Expand All @@ -537,10 +564,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");
}
Expand Down
Loading
Loading