-
Notifications
You must be signed in to change notification settings - Fork 100
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][CIRGen] Improve switch support for unrecheable code #528
Changes from 1 commit
94d428b
1ef7eab
d5b3cc1
8fb3356
28bf4ec
2fdc25b
1139cfe
cb09502
8d37db3
cb185ad
7d4101a
67da9e6
f88ee31
403ae40
b21e81e
ead25fe
be13b81
f387e1a
2bde4fe
6a0e742
1d442f6
0affd8d
3aeb3e0
006dfd0
67859c4
58ccd20
8fa0bbb
ada6175
d744421
50d414a
9cbc015
fbb25ae
b0dd640
21718ce
439217e
7463d87
d3e07d2
2d58e58
b91b91c
11024a5
e881544
5a6fe55
9989230
65880d7
2f48e4a
1ee5f18
92cceb7
1273f07
d98b9f0
0d91f68
bcfacd1
4aa8ff9
7a4e5c3
c003cb3
8e6cad9
b41ff5f
979ea99
4541cfa
7bd9fe3
bc1b7c9
7acd41f
e4f7839
3cf377a
1de9a22
8d26c92
1cd5a4b
d1d7b19
1390496
fae636d
1da7e33
000f9f7
e1ad7ee
3f9153d
a602024
c5e3476
eced6fa
efb9922
87c4338
765e5dc
58b5474
7b4745e
323e0c1
615a367
a010209
c35b579
d54e041
e7e05a8
6094902
da3343a
38bd933
113b787
cb53cf8
671ffc7
72e9f23
013a653
e95a5c4
c3e6388
4f89aa7
6c29cd4
06da08a
101c4c0
b63a943
ad2de47
877a15e
c976e4b
d7f09ca
a1d0b22
49bcdd2
6d83a86
d3b91b5
19350cf
e3d14fd
e0dc80f
b5e684e
da2e461
559da04
321ca2b
e846fd2
72eccae
b82fdb0
624f2be
876101b
c2bca4a
85a48c7
9a69667
0ba4f5a
116a86d
19de5fc
9d55d0f
6191da3
2ab8d1e
bfc16da
e10f9d9
62e97a7
062b69e
460cffa
cc4fddd
26e1ad1
1ee9340
829dd95
d7d62a6
108d233
47c2e56
65f27ad
a7910b8
c550e90
15903b6
2c680f1
90786c8
3d86b46
7c7c4ff
1f8f096
2a0fcd5
9fde505
3896c0e
fd354e5
8d87565
c9dd759
670f30d
49b609b
d18777b
e58a597
84851f8
4b49152
a853799
8ba2142
eaea071
a5c9aca
7f4768b
459f6f5
7804db0
ccd551b
06cf3c3
17c8db6
58b8bc7
90710ce
a0e3c1c
91685f8
6233ee0
a1bc344
906c314
7865351
84ebc8c
440f02e
12fb4ed
7380a63
275f0e4
267dd72
a53950f
62c6a86
2db13d8
730ffda
8b4c7e0
5348562
043f8dc
7740a8d
080f276
95e11a5
4c07a3c
4361c2d
c1311f5
dce1725
c0061fc
b5429f1
73da668
536d4c2
8daf005
1c3cd0a
9327912
8ef3b36
9c305cb
b51fcf7
b64cb19
5d9a961
b61cf01
77f4a0c
da1dd0f
68fe3ed
9daae1b
7402372
96722f7
7cda6e9
116e184
ded2fa7
5718db5
a1cb2c0
80f74eb
26a5b07
67fa2b7
426233f
ccddaa4
e197d4e
97b7280
09c86ed
c9b4fdc
9da804f
35f6ea5
bdfdb22
64a6edb
b361bbe
1efee91
795925f
5961a8d
f52e99e
981c976
f9b1067
f690ca9
00c75fa
9ae5d1f
7a61b3c
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 |
---|---|---|
|
@@ -24,26 +24,6 @@ using namespace cir; | |
using namespace clang; | ||
using namespace mlir::cir; | ||
|
||
namespace { | ||
|
||
// Handle stmt that don't belong to any cases. | ||
void checkCaseNoneStmt(const Stmt &S, bool checkCaseStmt = true) { | ||
if (S.getStmtClass() == Stmt::LabelStmtClass) | ||
llvm_unreachable("LabelStmtClass support NYI"); | ||
|
||
if (S.getStmtClass() == Stmt::CaseStmtClass || | ||
S.getStmtClass() == Stmt::DefaultStmtClass) | ||
assert(!checkCaseStmt && "CaseStmtClass/DefaultStmtClass support NYI"); | ||
|
||
if (S.getStmtClass() == Stmt::SwitchStmtClass) | ||
checkCaseStmt = false; | ||
|
||
for (const Stmt *c : S.children()) | ||
checkCaseNoneStmt(*c, checkCaseStmt); | ||
} | ||
|
||
} // namespace | ||
|
||
Address CIRGenFunction::buildCompoundStmtWithoutScope(const CompoundStmt &S, | ||
bool getLast, | ||
AggValueSlot slot) { | ||
|
@@ -348,6 +328,14 @@ mlir::LogicalResult CIRGenFunction::buildLabelStmt(const clang::LabelStmt &S) { | |
// IsEHa: not implemented. | ||
assert(!(getContext().getLangOpts().EHAsynch && S.isSideEntry())); | ||
|
||
// TODO: After support case stmt crossing scopes, we should build LabelStmt | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any |
||
// and clean LexicalScope::IsInsideCaseNoneStmt. | ||
for (auto *lexScope = currLexScope; lexScope; | ||
lexScope = lexScope->getParentScope()) { | ||
assert(!lexScope->IsInsideCaseNoneStmt && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if you remove this code? Also, why doesn't it work to just walk the scope up until you find a switch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Firstly, we won't need this assert anymore if we could keep the case none stmt somehow as you suggested.
Remove this code won't cause incorrect behavior currently (as we didn't support goto in that case yet), but I think it may produce strange error message in the future.
We need to avoid erasing the
Refer to the below code, we need to guarantee the removed
|
||
"LabelStmt inside case none stmt NYI"); | ||
} | ||
|
||
return buildStmt(S.getSubStmt(), /* useCurrentScope */ true); | ||
} | ||
|
||
|
@@ -479,6 +467,11 @@ mlir::LogicalResult CIRGenFunction::buildReturnStmt(const ReturnStmt &S) { | |
assert(!UnimplementedFeature::requiresReturnValueCheck()); | ||
auto loc = getLoc(S.getSourceRange()); | ||
|
||
// TODO: Rewrite the logic to handle ReturnStmt inside SwitchStmt, then | ||
// clean up the code below. | ||
if (currLexScope->IsInsideCaseNoneStmt) | ||
return mlir::success(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found many sample code that failed due to incorrect terminator in block, e.g.
Looks like it's another large work, so I just skip ReturnStmt here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, can you file a new issue and list these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm opposed to |
||
|
||
// Emit the result value, even if unused, to evaluate the side effects. | ||
const Expr *RV = S.getRetValue(); | ||
|
||
|
@@ -724,6 +717,22 @@ CIRGenFunction::buildSwitchCase(const SwitchCase &S, mlir::Type condType, | |
llvm_unreachable("expect case or default stmt"); | ||
} | ||
|
||
mlir::LogicalResult CIRGenFunction::buildCaseNoneStmt(const Stmt *S) { | ||
// Create orphan region to skip over the case none stmts. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because you are creating an orphan region, this mean that anything emitted inside a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't find a good place to hold the block of For example
There is no region inside Did I misunderstand something? Looking forward to your suggestions~ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand your point, but if you go for the current approach you might as well skip this codegen entirely, because what you are emitting won't ever be attached to anything. I think it's safer to mimic the original codegen here, what is Clang currently doing for OG codegen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should create a SwitchOp with at least one default region and delete that at the end if it ends up unused? |
||
mlir::Region region; | ||
auto *block = builder.createBlock(®ion); | ||
mlir::OpBuilder::InsertionGuard guard(builder); | ||
builder.setInsertionPointToStart(block); | ||
|
||
currLexScope->IsInsideCaseNoneStmt = true; | ||
|
||
auto res = buildStmt(S, /*useCurrentScope=*/isa<CompoundStmt>(S)); | ||
|
||
currLexScope->IsInsideCaseNoneStmt = false; | ||
|
||
return res; | ||
} | ||
|
||
mlir::LogicalResult | ||
CIRGenFunction::buildCXXForRangeStmt(const CXXForRangeStmt &S, | ||
ArrayRef<const Attr *> ForAttrs) { | ||
|
@@ -1004,7 +1013,7 @@ mlir::LogicalResult CIRGenFunction::buildSwitchBody( | |
builder.setInsertionPointToEnd(lastCaseBlock); | ||
res = buildStmt(c, /*useCurrentScope=*/!isa<CompoundStmt>(c)); | ||
} else { | ||
checkCaseNoneStmt(*c); | ||
buildCaseNoneStmt(c); | ||
continue; | ||
} | ||
|
||
|
@@ -1016,7 +1025,7 @@ mlir::LogicalResult CIRGenFunction::buildSwitchBody( | |
return res; | ||
} | ||
|
||
checkCaseNoneStmt(*S); | ||
buildCaseNoneStmt(S); | ||
return mlir::success(); | ||
} | ||
|
||
|
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 we need this, reasons below.