-
Notifications
You must be signed in to change notification settings - Fork 281
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
[SV][ETC] Fix perf bug removing from a SetVector of slice results. #5353
[SV][ETC] Fix perf bug removing from a SetVector of slice results. #5353
Conversation
On one design (-Odebug), ETC was taking 600s / 1100s total, after this change only takes ~20s. Just drop the `remove` entirely, as it'll avoid re-slicing through the same operation again (minor) and more to the point AFAICT we add the "rootOp" removed here back to the result sets in the end anyway.
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.
If it doesn't change the RTL for a large design, LGTM. The only thing I can think of might be related to keeping the ordering in the set such that it's insertion order in the extraction?
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 believe this is quite and old piece of code, and I'm not even sure if the "don't want it in the results" holds in the current implementation. We have pretty good unit test coverage here, as bugs have been fixed and edge cases supported, but also agree it would be best to make sure the result is unaffected on a real design.
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.
LGTM, thanks! This change doesn't change the output since getBackwardSlice
appends root operations anyway.
Yeah, as Lenharth said the iteration (=insertion) order could in theory be different re:adding the ops again at the end. Missed that. Diff'ing outputs on an internal design doesn't show any changed output, FWIW (output Verilog is 1.2GB). I /think/ this might (in some edge case where one slice requested adds itself or a root considered later to its slice, before adding all roots at the end) not produce identical output before/after but should still be deterministic for either before or after runs. Removing is a bit sketch anyway re:correctness of slice computation in current code, assumes either an empty result list incoming (not true here) or that the roots are always added to results afterwards (which we do), otherwise may remove one root that's part of another root's slice unexpectedly (earlier root adds root X to set, later root X removes itself). Anyway, that's all to say I think this is a step in the right direction and unlikely to cause problems or even differences in the output. |
Thanks for review, all! |
On one design (-Odebug), ETC was taking 600s / 1100s total, after this change only takes ~20s.
Just drop the
remove
entirely, as it'll avoid re-slicing through the same operation again (minor) and more to the point AFAICT we add the "rootOp" removed here back to the result sets in the end anyway.Removing an element from SetVector does not scale well.