-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[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.
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
Changes from 4 commits
ff564e6
ee0b0aa
3142924
b032de7
39861dd
c772736
906e697
f470023
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -253,6 +253,7 @@ mlir::LogicalResult CIRGenFunction::emitSimpleStmt(const Stmt *s, | |||||||||||
| case Stmt::NullStmtClass: | ||||||||||||
| break; | ||||||||||||
| case Stmt::CaseStmtClass: | ||||||||||||
| case Stmt::DefaultStmtClass: | ||||||||||||
| // If we reached here, we must not handling a switch case in the top level. | ||||||||||||
| return emitSwitchCase(cast<SwitchCase>(*s), | ||||||||||||
| /*buildingTopLevelCase=*/false); | ||||||||||||
|
|
@@ -428,6 +429,52 @@ mlir::LogicalResult CIRGenFunction::emitBreakStmt(const clang::BreakStmt &s) { | |||||||||||
| return mlir::success(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const CaseStmt *CIRGenFunction::foldCaseStmt(const clang::CaseStmt &s, | ||||||||||||
| mlir::Type condType, | ||||||||||||
| mlir::ArrayAttr &value, | ||||||||||||
| cir::CaseOpKind &kind) { | ||||||||||||
| const CaseStmt *caseStmt = &s; | ||||||||||||
| const CaseStmt *lastCase = &s; | ||||||||||||
| SmallVector<mlir::Attribute, 4> caseEltValueListAttr; | ||||||||||||
|
|
||||||||||||
| // Fold cascading cases whenever possible to simplify codegen a bit. | ||||||||||||
| while (caseStmt) { | ||||||||||||
| lastCase = caseStmt; | ||||||||||||
|
|
||||||||||||
| auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext()); | ||||||||||||
|
||||||||||||
| auto intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext()); | |
| APSInt intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext()); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use auto here, or 447.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (auto *rhs = caseStmt->getRHS()) { | |
| // If the case statement has an RHS value, it is representing a GNU | |
| // case range statement, where LHS is the beginning of the range | |
| // and RHS is the end of the range. | |
| if (const Expr *rhs = caseStmt->getRHS()) { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RHS is only valid for a GNU-range version of a case, do we have a test for that?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| auto endVal = rhs->EvaluateKnownConstInt(getContext()); | |
| APSInt endVal = rhs->EvaluateKnownConstInt(getContext()); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rangaes -> ranges
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // We may not be able to fold rangaes. Due to we can't present range case | |
| // We don't currently fold case range statements with other case statements. | |
| // TODO(cir): Add this capability. |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it deserves an assert on missing features?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to figure this out, but the check on line 465 guarantees that we will never get here unless this was the first case we are trying to fold. Can you add an assertion that verifies that? If the code is ever changed to make this untrue, we could easily lose cases.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So IIRC, 'anyof' is only valid if the case statements themselves are empty, right? Else they could contain a label for a GOTO, or be a duffs-device/etc. So i think we're being overly aggressive about joining these up here.
That said, i find myself wondering if the FE should be doing this sort of joining at all, rather than as a very early 'normalization' opt-pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be moved to an optimization pass. Perhaps CIRSimplify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cir-simplify isn't upstreamed yet should I go ahead and create it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we do it in a followup, but it might be nice to see that sort of thing happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm in this case, should I leave the current code and add a TODO for the follow-up work?
Because if we move this logic to a pass , that means the tests will need to change as well, since the anyof won't be emitted until after the simplify is upstreamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to just change the tests. The simplify pass shouldn't be run for the emit-cir tests unless it's explicitly added. There are a number of cases where we're doing optimization like this in the front end, and I really don't think we should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, @mmha has been working on cir-simplify and will be posting a PR soon, so you should wait for that to land and then add the case folding after it is in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're no longer folding cascading case statements, then this function could be simplified we wouldn’t need the while loop anymore since we're just extracting the kind and value from a single case. In that scenario, the assertion proposed by @andykaylor would no longer be necessary, because range cases would only ever be processed once at the top level.
const CaseStmt *CIRGenFunction::foldCaseStmt(const clang::CaseStmt &s,
mlir::Type condType,
mlir::ArrayAttr &value,
cir::CaseOpKind &kind) {
const CaseStmt *caseStmt = &s;
SmallVector<mlir::Attribute, 1> caseEltValueListAttr;
llvm::APSInt intVal = caseStmt->getLHS()->EvaluateKnownConstInt(getContext());
// If the case statement has an RHS value, it is representing a GNU
// case range statement, where LHS is the beginning of the range
// and RHS is the end of the range.
if (const Expr *rhs = caseStmt->getRHS()) {
assert(caseStmt == &s && "Range case must be the first case processed");
llvm::APSInt endVal = rhs->EvaluateKnownConstInt(getContext());
SmallVector<mlir::Attribute, 4> rangeCaseAttr = {
cir::IntAttr::get(condType, intVal),
cir::IntAttr::get(condType, endVal)};
value = builder.getArrayAttr(rangeCaseAttr);
kind = cir::CaseOpKind::Range;
// We don't currently fold case range statements with other case statements.
// TODO(cir): Add this capability.
assert(!cir::MissingFeatures::foldRangeCase());
return caseStmt;
}
caseEltValueListAttr.push_back(cir::IntAttr::get(condType, intVal));
if (!caseEltValueListAttr.empty()) {
value = builder.getArrayAttr(caseEltValueListAttr);
kind = cir::CaseOpKind::Equal;
}
return caseStmt;
}We don’t need the while loop anymore because we must break as soon as we encounter a cascading case or default statement.
// Break early if we found a range. We can't fold ranges.
// Also break if we found a cascading case/default.
if (caseStmt) {
const Stmt *sub = caseStmt->getSubStmt();
if (caseStmt->getRHS() || isa<CaseStmt>(sub) || isa<DefaultStmt>(sub))
break;
}Also, given that we're no longer folding multiple cases, I think we should consider renaming the function to better reflect its new behavior. Something like getCaseInfo ?
Is my approach correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say you don't even need to call a function at all. You can go back to what you had before in emitCaseStmt but with special handling added for the GNU range case.
There was a problem hiding this comment.
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