onnx-mlir
onnx-mlir copied to clipboard
onnx
Mixing compiler and pass options
Hi, folks: @tungld, @sstamenova, and @etiotto.
I'm looking at PR #1269 and how its interacting with my team's usage of onnx-mlir. We use the passes of ONNX-MLIR without picking up the onnx-mlir/onnx-mlir-opt binaries.
After this change we're running into issues because linking withe some of the passes now pulls in all of the command-line options for onnx-mlir, which we don't want.
i've looked at couple of options for this and i'd like to either a) move the pass-specific flags to a single lib (e.g., so we have PassOptions and CompilerOptions as sperate things) b) locate the pass specific flags into the passes themselves. c) remove the idea of pass specific flags and have that information passed to the passes on their construction
Option (a) seems like the least invasive while option (b) seems like the most flexible.
Thoughts?
To rephrase your situation, you want to use some of the onnx-mlir passes but are not planning to use either the onnx-mlir or onnx-mlir-opt drivers. And you are stating that when you do that, you have options that are added (via the llvm support for options) that are not desired.
Hi, @AlexandreEichenberger , thanks for responding. Your rephrasing captures the issue well. To give a concrete example, and make this less abstract: i have some name clashes which are the major issue (otherwise hiding them would be fine). For example, as you might imagine, my tool also has an -mtriple option.
The onnx-mlir sources make a clear distinction between pass options and tool options; unfortunately, after the aforementioned PR, they're now included together.
These pass options are actually not "real" pass options. They are actually compiler options and passes access them globally.
This is one example of how to define a pass option: https://github.com/onnx/onnx-mlir/blob/main/src/Conversion/ONNXToKrnl/ConvertONNXToKrnl.cpp#L172
@tungld , yes that is correct. a different design might be to pass the command-line options to the passes; i can take that route as well if its more appealing to folks. its a bit more invasive though.
My PR #1348 is green now. Any thoughts from folk on if this is the right direction or one of the other 3 options?
a) move the pass-specific flags to a single lib (e.g., so we have PassOptions and CompilerOptions as sperate things) what is implemented ==> b) locate the pass specific flags into the passes themselves. c) remove the idea of pass specific flags and have that information passed to the passes on their construction d) add actual pass options and remove pass-specific command-line options
@manbearian I am fine that PR #1348 can be a short-term solution to make your usage of onnx-mIir work. For a long term solution, I would prefer to make the passes self-contains by having their own options. Information should be passed via construction.
i'll look at the alternative change. i might be able to get it done today; no promises though.
@tungld, please take a look at PR #1363 and let me know if this what you're thinking.
please take a look at PR https://github.com/onnx/onnx-mlir/pull/1363 and let me know if this what you're thinking.
@manbearian Thank you for quickly implementing the idea! Yes, it is what I am thinking about. I would go with PR #1363.
@AlexandreEichenberger , i see you also approved #1348 . What's your take on #1348 vs #1363. i think i agree with @tungld that the latter is better.
I now realize that they were two versions for the same original issue. After reviewing the concepts behind #1363, you convinced me that that one is better. Thanks for having followed up on @tungld 's suggestions, much appreciated.
@AlexandreEichenberger or @tungld , thanks for the feedback. Can one of you please drive the commit to completion? i don't have permission to run tests or merge changes
I believe we have some restrictions to run CIs on our internal machines, once its approved and ran successfully, I think you can merge it. But we can do that too.