C++/C#: generate IR for funcs excluded in PrintIR #2975
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, functions excluded from PrintIR would not have IR generated. This sometimes affected escacpe analysis of functions that were printed.
|
We could still have the exclusion in place for |
|
@jbj We could, but I'm not sure it's worth making the PrintIR queries diverge depending on the IR flavor. |
dbartol
previously approved these changes
Mar 4, 2020
dbartol
reviewed
Mar 4, 2020
There was a problem hiding this comment.
Wait a minute. @rdmarsh2, can you compare PrintIR performance before and after on Wireshark, printing a single function? One of the original motivations for skipping IR construction was to reduce the cost of computing toString() for every instruction, opera day, etc.
|
I can. I'm sure it will be much slower, but my personal preference would be to speed it up explicitly rather than have implicit differences between the printed IR and the IR that queries are using. |
|
Looks like it still tries to generate strings for everything... I let it run for 2 hours and it stalled out in a toString predicate. |
|
@rdmarsh2 Maybe some manual magic on the relevant string predicates would help? We really only need those strings for the IR that we're actually printing. I'm certainly willing to pay the price of constructing all of the IR itself, even if that's significant. |
|
I'd hoped to get buy-in for evaluating the whole IR during yesterday's meeting but we didn't get to it. I'll go ahead and experiment with manual magic. |
|
Timings on Wireshark to dump IR for a single function with the new changes, after IR evaluation: |
dbartol
approved these changes
Mar 24, 2020
jbj
reviewed
Mar 25, 2020
| @@ -307,13 +310,20 @@ class Instruction extends Construction::TInstruction { | |||
| result = getResultString() + " = " + getOperationString() + " " + getOperandsString() | |||
| } | |||
|
|
|||
| predicate shouldGenerateDumpStrings() { | |||
There was a problem hiding this comment.
Suggested change
| predicate shouldGenerateDumpStrings() { | |
| private predicate shouldGenerateDumpStrings() { |
There was a problem hiding this comment.
Oops, missed this before I merged. I'll fix in another PR immediately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, functions excluded from PrintIR would not have IR
generated. This sometimes affected escape analysis of functions that
were printed.