This simplifies a few parts of the pass manager, but in particular we don't add as many
verifierpass as there are passes in the pipeline, and we can now enable/disable the
verifier after the fact on an already built PassManager.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
You should probably note somewhere that this changes the default behavior of the pass manager to not verify passes by default.
mlir/lib/Pass/Pass.cpp | ||
---|---|---|
359 | unrelated nit: This should just return the error directly. | |
393 | Should we hoist this above the instrumentation calls? I'm inclined to say yes. | |
394 | Why? Do we intend to treat cases where a pass accidentally changes something outside of the current operation as UB? It'd still be nice to get verifier failures when my, e.g., function pass does something that it shouldn't. | |
483 | Is there a double space after 'on'? | |
512 | unrelated nit: Can you remove this auto while you are here? | |
745–748 | ? |
mlir/lib/Pass/Pass.cpp | ||
---|---|---|
393 | I wasn't sure, the current version preserve the existing behavior I believe, but I'm happy to move it above as well! Wouldn't it mess up things like pass timing by including the verifier into it? | |
394 | Not sure I understand? I added this check because I though that the last pass in the OpToOpPassAdaptor pipeline would already have verified the IR. Is this a nesting question: an extra verifier at this level would check a level above maybe? |
mlir/lib/Pass/Pass.cpp | ||
---|---|---|
393 |
Yeah, I think it goes either way. I would expect several instrumentations to benefit from the fact that the pass fails when the verifier fails, i.e. it would be easier to track down which pass caused a specific failure. Pass instrumentation would hurt more, but I think that one can be explained away as similar to "do you profile in debug mode?". If pass timing is the one negative hit, which was the only one I could think of, then I'd want to move it. Unless you can think of others that might be negatively impacted? | |
394 | Sorry, yes it is a nesting thing. I'd still want us to verify the parent module after running a nested pipeline on it. |
mlir/include/mlir/Pass/PassManager.h | ||
---|---|---|
152 | This comment needs to be updated. |
mlir/lib/Pass/Pass.cpp | ||
---|---|---|
790 |
Did you miss the default value for verifyPasses here? |
mlir/lib/Pass/Pass.cpp | ||
---|---|---|
790 | I most certainly did. |
This comment needs to be updated.