-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add expression lowering lit based regression tests #41
Conversation
- Modify bbc so that it can output mlir on the standard output so that it can be piped. - Modify `fir::createFuncOp` so that it create new function after existing functions. It prevented testing with FileCheck of file with several functions, and in general seems reasonable. - Add integer, real, and logical intrinsic operations (but pow) lowering tests.
@@ -0,0 +1,12375 @@ | |||
! Generated test file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to checkin the generated output here or have a test the generates the output as part of the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the latter point: one can build up a "shell script" with multiple RUN lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this should not be checked in, it was just for review purpose. I liked the idea of having the file generated somewhere though for easier failure debug, but the "shell script" option might also be good with –dump-input=fail
.
} else { | ||
errs() << "oops, pass manager reported failure\n"; | ||
mlirModule.print(out.os()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a little off to me. The -emit-llvm
flag is meant only to enable writing out the file.ll
with the LLVM IR. It wasn't meant to enable and disable all the passes the pass manager runs. Each of those should have their own on/off option individually. Or, if we want to separate out "optimizations", they might be under a -O
flag of some sort. Getting both the LLVM IR and the MLIR dump as separate outputs from a single run of bbc
seems desirable. Also, since the focus of bbc
is testing Burnside, the LLVM IR output was to be opted into, but "lowering" should encompass more than just the bridge, I think. We want to run a few standard passes to make sure what the bridge is producing isn't garbage IR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I made a small update adding a switch for llvm IR output filename and to control FIR passes independently of LLVM IR generation.
I am actually not sure what passes are required lowering steps and which one are "optimizations" (can be skipped to generate LLVM), so I use "--fir-passes" switch instead of "--optimize". Which passes are required if one then wants to lower to LLVM ?
it can be piped.
fir::createFuncOp
so that it create new function after existingfunctions. It prevented testing with FileCheck of file with several
functions, and in general seems reasonable.
tests (they are generated, I plane to have them generated by CMake instead of having them in the source, but for this review, I find it nicer to make the tests explicit).