C++: add support for GNU StmtExpr in IR#984
Conversation
73250dc to
b39d985
Compare
dave-bartolomeo
left a comment
There was a problem hiding this comment.
Just a couple minor but important issues.
| } | ||
|
|
||
| override Instruction getChildSuccessor(TranslatedElement child) { | ||
| result = getParent().getChildSuccessor(this) |
There was a problem hiding this comment.
This needs and child = getStmt() to avoid holding for all TranslatedElements in the snapshot.
There was a problem hiding this comment.
@rdmarsh2 Given that this went unnoticed, please make sure you test these changes for performance on a snapshot that uses many statement expressions. The heaviest user I know of is the Linux kernel.
| } | ||
|
|
||
| override Instruction getResult() { | ||
| result = getTranslatedExpr(expr.getResultExpr()).getResult() |
There was a problem hiding this comment.
| result = getTranslatedExpr(expr.getResultExpr()).getResult() | |
| result = getTranslatedExpr(expr.getResultExpr().getFullyConverted()).getResult() |
| return a[0] + fn(1.0); | ||
| } | ||
|
|
||
| int ExprStmt(int x) { |
There was a problem hiding this comment.
Please add another test case with multiple statements and some control flow within the StmtExpr.
| # 1002| 1: return ... | ||
| # 1002| 0: (statement expression) | ||
| # 1002| Type = int | ||
| # 1002| ValueCategory = prvalue |
There was a problem hiding this comment.
It looks like PrintAST doesn't support StmtExpr. That's not a blocker for this PR.
dave-bartolomeo
left a comment
There was a problem hiding this comment.
LGTM, but please check performance on Linux kernel snapshot as suggested by @jbj.
427da67 to
4f5dd2a
Compare
|
Benchmarked - it's about a 7% slowdown on the Linux kernel. Over 20% of functions in the kernel have a statement expression included, which means that without this PR, portions of the function are unreachable in the raw IR and therefore don't have later stages of IR generation performed. Given that, the slowdown actually seems smaller than I would have guessed. I'll get the conflicts fixed tomorrow. |
|
7% sounds like a lot. Does it mean 7% time added to the query Are there any predicates or stages that stand out as contributing the majority of this 7% slowdown? How does performance change on a big snapshot that doesn't use |
|
The number of instructions in the final IR goes from 6,538,958 to 8,986,093 - an increase of about 37%. On master, raw IR generation is performed for the contents of the |
|
Testing on Wireshark shows a 3% increase in instructions in the final IR, and no meaningful change in evaluation time (2 second increase on a 40 minute run) |
4f5dd2a to
46f93ff
Compare
dave-bartolomeo
left a comment
There was a problem hiding this comment.
LGTM once the test failures are fixed
jbj
left a comment
There was a problem hiding this comment.
Thanks for verifying performance.
|
That test failure actually shows a potential issue. We don't generate an instruction for the StmtExpr, and instead just pass the result instruction of the last expression to the parent of the StmtExpr. That means the selected location doesn't quite match the old data flow library. We could potentially insert a copy instruction to make that location appear. |
|
Inserting a copy instruction sounds good. I agree we should aim to map each AST element to a unique IR element. |
No description provided.