core: add MLIROptPass, calling mlir-opt #1257
Conversation
superlopuh
force-pushed
the
sasha/mlir-opt-pass
branch
from
b93f070
to
de7b2b2
Compare
superlopuh
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1257 +/- ##
==========================================
+ Coverage 89.34% 89.41% +0.06%
==========================================
Files 181 182 +1
Lines 24080 24119 +39
Branches 3663 3666 +3
==========================================
+ Hits 21515 21565 +50
+ Misses 1991 1980 -11
Partials 574 574
☔ View full report in Codecov by Sentry. |
| if not shutil.which("mlir-opt"): | ||
| raise ValueError("mlir-opt is not available") |
There was a problem hiding this comment.
Does this work platform-independently?
There was a problem hiding this comment.
I have no idea :) what platforms should I test this on?
There was a problem hiding this comment.
Not sure, not even sure we need somethgin else than linux, we aren't weirdos after all.
Maybe documentation is enough? Also, from looking at the docu, shutil should at least work relatively well with several platforms, so....
There was a problem hiding this comment.
I believe we should just keep it as it is for now, and then fix it if it later on becomes a problem ? Also, it seems it should work in Python.
| @@ -0,0 +1,15 @@ | |||
| // RUN: xdsl-opt %s -p mlir-opt{arguments='--cse','--mlir-print-op-generic'} --print-op-generic | filecheck %s | |||
There was a problem hiding this comment.
Not sure how mlir structures it's optimizations, but this feels more like dce than cse. Maybe change the example (or ignore the comment 😉 )
There was a problem hiding this comment.
mlir-opt: Unknown command line argument '--dce'. Try: 'mlir-opt --help'
mlir-opt: Did you mean '--cse'?
There was a problem hiding this comment.
cse does dce if I recall, there is no dce passes AFAIK?
| // CHECK: "builtin.module"() ({ | ||
| // CHECK-NEXT: "func.func"() ({ | ||
| // CHECK-NEXT: "func.return"() : () -> () | ||
| // CHECK-NEXT: }) {"function_type" = () -> (), "sym_name" = "do_nothing"} : () -> () | ||
| // CHECK-NEXT: }) : () -> () |
There was a problem hiding this comment.
Why not use CHECK-NOT? Feels like it's intended for exactly this. Plus. this will be more stable to printing changes for all of these ops.
There was a problem hiding this comment.
I like check-next for the whole thing because then if it prints an error or something unexpected then at least we catch it. I'm not sure how robust check-not would be if mlir-opt prints "error"
| @@ -0,0 +1,15 @@ | |||
| // RUN: xdsl-opt %s -p mlir-opt{arguments='--cse','--mlir-print-op-generic'} --print-op-generic | filecheck %s | |||
There was a problem hiding this comment.
cse does dce if I recall, there is no dce passes AFAIK?
| if not shutil.which("mlir-opt"): | ||
| raise ValueError("mlir-opt is not available") |
There was a problem hiding this comment.
I believe we should just keep it as it is for now, and then fix it if it later on becomes a problem ? Also, it seems it should work in Python.
|
This adds a Python class to pipe code through
mlir-opt. This has been quite useful to me while writing the pass pipeline in Python, to leverage things like cse and lowering through builtin dialects. I'm planning to take mlir-opt out of the Toy tutorial for the sake of having an end-to-end compiler with no dependencies, but I think it will be useful to future projects using xDSL.