-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Define different function for concrete playback (no user impact) #2407
Conversation
It looks like the `if cfg!()` is no longer being propagated and the concrete playback code is increasing the logic that the compiler detects as reachable.
Notes on the alleged performance regression: The resulting GOTO binaries are (as one should expect from the changes in this PR) the same, with one exception: the numbers in Kani-generated
I have highlighted the (only significant) differences. So unless we find a way to make Kani's choice of names deterministic there might not be much we can do to also have deterministic SAT solver performance. |
Thanks @tautschnig for your thorough analysis. That's exactly what I was afraid of. Not sure what's the best course of action. Any suggestions? |
Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
As per the discussion that happened out of band in the meantime: let's keep collecting data for the time being, no obvious reason that this specific PR ought to be blocked. |
Description of changes:
Split the definitions of functions that have specific logic for concrete playback to avoid generating code for unused logic.
Background: While upgrading the toolchain to nightly-2023-04-16, I noticed a few new warnings regarding caller location and the size of the model was much bigger in some case. The MIR file was going from a couple hundreds of lines to 20k+ lines.
It looks like the
if cfg!()
is no longer being propagated and the concrete playback code is increasing the logic that the compiler detects as reachable.Resolved issues:
Related to #2383 and PR #2406
Related RFC:
Optional #ISSUE-NUMBER.
Call-outs:
We might want to split the code like we've done with the macro one to make it a bit easier to maintain but for now, I'm jut unblocking the toolchain update.
Testing:
How is this change tested? Current tests
Is this a refactor change? Yes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.