[Flang][bbc] Prevent bbc -emit-fir command invoking all passes twice #80927
Conversation
Currently when the bbc tool is invoked with the emit-fir command the pass pipeline will be invoked twice causing passes added to the pass manager to run twice on the input IR. This change seeks to prevent that from occuring by only invoking the pass managers run command once and situationally adding the HLFIR to FIR pass pipeline in the same scenario as before.
There was a problem hiding this comment.
Hi, thanks for looking into this.
Only the verifier pass is invoked twice. You can see this using bbc --mlir-pass-statistics.
I don't think this is the right fix. This patch would cause the verifier pass not to be run at all when -emit-hlfir is used. I think you should bring the second pm.run(mlirModule) out of the if statement.
Thank you for the option to check the passes being ran! It seems that it is restricted to the OpenMP passes being ran twice due to the location: https://github.com/llvm/llvm-project/blob/main/flang/tools/bbc/bbc.cpp#L378 so perhaps shifting the location these are added is the correct fix. |
…s twice" This reverts commit 3fd8e67.
This is done by invoking them in a seperate pass manager as soon as neccesary. The alternative would be to place a clear in-between the two invocations of run and re-add the verification pass, but this seems like a better way to seperate concerns and keep future logic addiitons simpler.
|
Updated the PR, I opted to break off the OpenMP pass pipeline into it's own function/pass manager to be executed at the point where I believe it's required (after the module generation and prior to the first verification pass). I opted to do it this way as opposed to inserting a pass manager clear and then re-add of the verification pass to the pass manager in-between the runs as this hopefully makes the logic a little simpler to follow and makes any new additions easier as well (effectively keeping the OpenMP logic encased in it's own world people can for the most part ignore). I am open to other alternatives though, but this seemed the simplest (that I could think of at least). |
There was a problem hiding this comment.
Thanks for the changes! Yes I think you have the OpenMP passes in the right place - they should run right after lowering.
I wonder if we should have a test for the OpenMP pass pipeline similar to flang/test/Driver/mlir-pass-pipeline.f90. I'm fine with that remaining out of the scope of this PR though as there apparently wasn't any testing before your changes.
Thank you very much for your help with the PR and the review! Yes, we likely should have added a test like that and it's likely important to add if we end up with more OpenMP specific passes. |
Currently when the bbc tool is invoked with the emit-fir command the pass pipeline will be invoked twice causing passes added to the pass manager to run twice on the input IR.
This change seeks to prevent that from occuring by only invoking the pass managers run command once and situationally adding the HLFIR to FIR pass pipeline in the same scenario as before.