[FIRRTL][LowerClasses] Improve performance, NFC #7060
Conversation
| @@ -447,7 +602,7 @@ LogicalResult LowerClassesPass::processPaths( | |||
| } | |||
|
|
|||
| // If we are at a leaf, nothing to do. | |||
| if (std::distance(it->begin(), it->end()) == 0) { | |||
| if (it->begin() == it->end()) { | |||
There was a problem hiding this comment.
llvm::size(A, B) exists specifically to confidently do std::distance(..) on ranges, just related tip.
For checking distance of zero, comparison seems good to me! 👍
uenoku
force-pushed
the
dev/hidetou/lower-class
branch
from
b7b104d to
915f5ac
Compare
| // Fill `shouldCreateClassMemo`. | ||
| for (auto moduleLike : circuit.getOps<FModuleLike>()) | ||
| shouldCreateClassMemo.insert({moduleLike, false}); | ||
| parallelForEach( |
There was a problem hiding this comment.
Please also check the single-threaded performance. Parallelization only improves walltime. We need to improve absolute time also.
There was a problem hiding this comment.
I checked the single threaded performance on a large internal design, and this change leads to a >90% reduction in runtime in a single threaded context.
youngar
changed the title
| if (isa<PortAnnoTarget>(target)) | ||
| // Port annotations are batch-updated afterwards. | ||
| portAnnotations.push_back(annotations.getArrayAttr()); | ||
| else | ||
| // Otherwise it's fine to update immediately. | ||
| target.setAnnotations(annotations); |
There was a problem hiding this comment.
target.setAnnotations(annotations); could be made slightly faster with cast<OpAnnoTarget>(target)setAnnotations(annotations);
I don't love something about the way this sometimes mutates an internal structure and other times just sets the annotations. I think I would be happier if this function always appended to an annotation array, and op modifications happened outside this function, and then portAnnotations doesn't need to be a member anymore. It would be even nicer if this whole function didn't care about the target type at all - but we need to know when we create the targetSym. WDYT?
There was a problem hiding this comment.
I changed processPathTrackers to return FailureOr<AnnotationSet> and mutate annotations from callers. Does that look good to you?
uenoku
force-pushed
the
dev/hidetou/lower-class
branch
from
4a5e2e1 to
fdea037
Compare
uenoku
force-pushed
the
dev/hidetou/lower-class
branch
from
fdea037 to
6380103
Compare
| return true; | ||
|
|
||
| // Create a class for modules with property ports. | ||
| bool hasClassPorts = llvm::any_of(moduleLike.getPorts(), [](PortInfo port) { |
There was a problem hiding this comment.
Just thinking out loud -- doing this in parallel when it walked the module body made sense, but maybe less so to walk the ports + instance nodes for each?
Something like:
"has property ports" can be computed once per module (and looked up instance-side, instead of re-computed, so tracked in a (temporary?) data structure), and "shouldCreateClassImpl" can then be determined in a single (serial) bottom-up walk of instance graph, computing "hasPropertyPorts" for module definitions along the way.
WDYT?
There was a problem hiding this comment.
I think that's great idea. We have to fix InstanceGraph to walk entire modules/classes for bottom-up walk so let me first merge the PR. I'll follow-up to use a bottom-up walk.
This PR fixes a regression caused by PortAnnotation mutation. We have to batch-update the port annotations to avoid compile time regression. Several other improves are added along with the fix:
shouldCreateClassis slightly expensive since it walks entire IR to check instances.shouldCreateClassis already lazily evaluated but howevershouldCreateClassis immediately called for every module. So this PR changes to compute the all results parallelly beforehand instead of lazy evaluation.std::distance(a, b) == 0is replaced witha == bto avoid potential iterator traversals (maybe it is optimized by clang/gcc though).processPathTrackersis refactored into a helper structPathTrackerso that we can process paths/annotations parallely. We updatepathInfoTablesequentially afterwards.It should be NFC completely. Pass should behave exactly the same way.