[firtool] Only run verifiers at required points in the pipeline#9559
[firtool] Only run verifiers at required points in the pipeline#9559
Conversation
The MLIR PassManager defaults to verifyPasses=true, which means verification runs after every pass by default. This change explicitly disables the built-in pass manager verification when not in "all" mode, allowing verification to run only when required. With this change, the three verification modes work as follows: - `--verify=all`: Uses MLIR's built-in pass manager verification (runs mlir::verify() after every pass, with optimization to skip when analyses are preserved) - `--verify=default`: Disables built-in verification, but runs custom verifier passes at strategic points in the pipeline where verification is required to catch user errors - `--verify=none`: Disables all verification (both built-in and custom)
|
Note: most of my concerns are about the use of this option as opposed to the existence of the option itself. I like the option existing. 👍 I'm a little spooked by this. When I suggested running with verify-each=none in the past we decided against that as we do want the IR verified to be correct all the time. There is also a safety concern here for internal use as running without verification could introduce bugs. This at least deserves some thought. How much do we save do we save by doing this, at each level of unsafe verification? I would rather put effort into making the verifiers fast and/or changing IR structures to make verification easier. Have you looked into any performance issues in existing verifiers? @darthscsi, WDYT? |
Based on my profiling, I'm skeptical that we can significantly reduce the runtime overhead here. The bulk of the cost comes from fundamental operations—specifically, walking the ilist for every operation and verifying dominance—rather than dialect-specific logic. Short of simply having less IR or fewer passes, it seems these structural costs are unavoidable with the current design. We can take a look at a profile together if you want to see if you can spot some areas for improvement. |
|
I must admit that I always add |
Is this due to certain large modules or is it just due to IR size generally, specifically region size? I had thought we were seeing lots of problems with very large single-region OM-related classes which was the primary driver in the slow dominance computation. Some thoughts:
The |
|
I think the question of whether to turn off verifiers in production is orthogonal to the question of how we can make verifiers faster. Similar to how we disable asserts in production, it is not normal to ship with verifiers permanently enabled. It seems like you are hoping that we can reduce verifier time to the point where the answer does not matter. While that is a good goal, perhaps we can decouple it from this change. Can we merge this PR, but with the default changed to keep running verifiers? That way, we maintain the current safety profile for general users. Then, for @uenoku's use case, he can explicitly use this flag to improve speed, which is slightly safer than disabling all verifier runs entirely. We can then move the discussion on how to speed up verification (and the specific dominance/region issues) to a new issue. |
|
How about this: let's at least add upstream's What may also be less controversial is to put
I don't think this is correct for either our flows, MLIR, or other MLIR-derived projects. Upstream guidance says verifiers must be fast, there was prior commentary about preferences to keep them enabled, and the default setting for When it was discussed internally to change the default for our flows in the past (2022 was when I wanted to make the change to
That is the dream! It was also the conclusion the last time this kind of thing was brought up.
More generally: Is this for developers and would they use it over This one would be good to have a meeting about and an addition to the rationale with the summary of the conclusions about our stance here. |
3 participants
The MLIR PassManager defaults to verifyPasses=true, which means verification runs after every pass by default. This changes the default behaviour in firtool and explicitly disables the built-in pass manager verification when not in "--verify=all" mode, allowing verification to run only when required by a pass.
With this change, the three verification modes work as follows:
--verify=all: Uses MLIR's built-in pass manager verification (runs mlir::verify() after every pass, with optimization to skip when analyses are preserved)--verify=default: Disables built-in verification, but runs custom verifier passes at strategic points in the pipeline where verification is required to catch user errors--verify=none: Disables all verification (both built-in and custom)The current pass pipeline is at the bottom of the description, annotated with where I have inserted verifier runs. I would like for people to review the pass list to see if anything requires verification and does not currently have it.
VerifyInnerRefNamespaceandVerifyObjectFieldsare listed as "guarded", which means they are enabled at only theAllsetting. I am not sure whatVerifyClockedAssertLikePassis, and but it should probably be grouped up with the aforementioned passes.One change is that the FIRRTL parser no longer runs the verifiers after parsing - it is now up to the caller to verify the IR after parsing.
circt-translate --import-firrtlwill still run IR verifiers after importing.Preliminary measurement on the effect of this change shows that 40% reduction in run time, but a negligible reduction in maximum rss.