-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CIR] Add if statement support #134333
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
[CIR] Add if statement support #134333
Changes from 3 commits
a91edc3
eba4924
25dff99
c997d6c
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 |
|---|---|---|
|
|
@@ -1484,6 +1484,20 @@ mlir::Value ScalarExprEmitter::VisitUnaryLNot(const UnaryOperator *e) { | |
| return {}; | ||
| } | ||
|
|
||
| /// 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( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 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.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
This isn't used now. It can be removed.