[FIRRTL][InferResets] Peel interesting operations paralelly, NFC #5629
Conversation
This commit reduce executin time of InferReests by 30%~40%. Since it's costly to walk the entire IR before Dedup, it's low hanging fruit to peel interesting operations (operations with uninferred resets in this case) parallely. Before: 54.5713 ( 1.5%) 54.5713 ( 7.8%) InferResets After: 34.2072 ( 0.9%) 34.2072 ( 5.1%) InferResets
| op->getResultTypes(), | ||
| [](mlir::Type type) { return typeContainsReset(type); }) || | ||
| llvm::any_of(op->getOperandTypes(), typeContainsReset)) | ||
| e.second.push_back(op); |
There was a problem hiding this comment.
Is it thread safe to update the vector moduleToOps in parallel ? If it is, then this is great.
There was a problem hiding this comment.
I think it's thread safe because moduleToOps[i] is mutated only by i-th iteration. That's why moduleToOps is constructed beforehand.
There was a problem hiding this comment.
This should be safe. Another option here since the result set is just aggregated is to use a parallel reduce. This is more efficient though, just slightly less clear (perhaps).
There was a problem hiding this comment.
LGTM. 2 things for possible future work:
- the walk IR and collect ops to worklist is a common pattern which we should factor out.
- the traceResets might be parallizable with a reduction over all the partial (per-module) results (thus parallelizing the main loop rather than the identification loop)
| if (typeContainsReset(e.type)) | ||
| return true; | ||
| return false; | ||
| static bool typeContainsReset(Type type) { |
There was a problem hiding this comment.
Feel free to peel this off and commit it directly.
| op->getResultTypes(), | ||
| [](mlir::Type type) { return typeContainsReset(type); }) || | ||
| llvm::any_of(op->getOperandTypes(), typeContainsReset)) | ||
| e.second.push_back(op); |
There was a problem hiding this comment.
This should be safe. Another option here since the result set is just aggregated is to use a parallel reduce. This is more efficient though, just slightly less clear (perhaps).
There was a problem hiding this comment.
LGTM! Very cool to see that significant speedup.
For some of these inference passes I'm wondering if there's a mode where we run the module-local part of the pass in parallel (e.g. figuring out the reset networks within a module), and then do a global pass across the modules afterwards. Not sure that will help much here because you're already getting a lot of mileage out of the parallelization 😎
| if (typeContainsReset(e.type)) | ||
| return true; | ||
| return false; | ||
| static bool typeContainsReset(Type type) { |
|
Yeah I think it does makes sense to accumulate module-scope equivalence relation paralelly and apply reductions. I originally went in that direction, but I changed to the current simpler implementation as the number of interesting operations is very small (there are 2*10^5 interesting ops out of 10^8 ops) so I guess the performance benefit might not be that much. |
This commit reduces the execution time of InferResets by 30%~40%. Since it's costly to walk the entire IR before Dedup, it's low hanging fruit to peel interesting operations (operations with uninferred resets in this case) paralelly.
Before:
54.5713 ( 1.5%) 54.5713 ( 7.8%) InferResets
After:
34.2072 ( 0.9%) 34.2072 ( 5.1%) InferResets