-
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 5 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); | ||||||||||||
|
|
@@ -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)) || | ||||||||||||
|
||||||||||||
| // we prefer generating |
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.
Right, I get that. I'm suggesting a better alternative suggestion for this and the next line.
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.
Oh, okay I’ll change it
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.
Nit: I'd prefer not to have this empty line here.
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.
DONE
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.
| value = builder.getArrayAttr(rangeCaseAttr); | |
| value = builder.getArrayAttr({ | |
| cir::IntAttr::get(condType, intVal), | |
| cir::IntAttr::get(condType, endVal) | |
| }); |
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.
DONE
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.
Do we need this? I thought we were going to move all of this to an opt pass instead?
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 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?
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 don't have a good idea? perhaps in the pass manager or whatever pass we expect to do this?
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 updated the comment to mention implementing this in CIRSimplify.
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.
I think you can do this to eliminate the caseEltValueListAttr variable.
value = builder.getArrayAttr({cir::IntAttr::get(condType, intVal)});
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.
DONE
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