Page MenuHomePhabricator

[flang][fir] Add fir-opt tool
ClosedPublic

Authored by clementval on Feb 11 2021, 12:01 PM.

Details

Summary

This patch introduce the fir-opt tool. Similar to mlir-opt for FIR.
It will be used in following patches to test fir opt and round-trip.

Diff Detail

Event Timeline

clementval created this revision.Feb 11 2021, 12:01 PM
clementval requested review of this revision.Feb 11 2021, 12:01 PM

Add missing new line

Thanks, I wrote exactly this locally before seeing this patch! :)

Can you also update test/Fir/fir-ops.fir and test/Fir/fir-types.fir to use this? I had changed the run line this way:

// Parse operations and check that we can reparse what we print.
// RUN: fir-opt %s | fir-opt | FileCheck %s
flang/tools/fir-opt/CMakeLists.txt
22

This list of dependency should be pruned.

These two are obvious:

FIROptimizer
MLIROptLib

None of the others are.
I suspect the issue is that fir::registerFIRPasses() should not just be an inline function in a header but would better come in an implementation file, with a CMake target that pulls in the minimum number of dependencies to link the desired passes.
Otherwise any user of this function has to know the list of targets, and changing the function requires to change the build configuration of every user.

flang/tools/fir-opt/fir-opt.cpp
24

Can you replace the two lines above with fir::registerFIRDialects(registry); ?

26

(nit: missing newline)

clementval marked 2 inline comments as done.Feb 11 2021, 1:12 PM

Thanks, I wrote exactly this locally before seeing this patch! :)

Thanks for the review.

Sorry, I was a bit slow to submit the patch. When I saw your comment I suspect you might have started to work on it on your side as well.

flang/tools/fir-opt/CMakeLists.txt
22

I have pruned it to a minimum. I'll check with @schweitz so we refactor fir::registerFIRPasses().

Address review comment

clementval marked an inline comment as done.Feb 11 2021, 1:12 PM
mehdi_amini added inline comments.Feb 11 2021, 1:31 PM
flang/tools/fir-opt/CMakeLists.txt
22

LG, please separate ${dialect_libs} and MLIRAffineToStandard after a line TODO: these should be transitive dependencies from a target providing "registerFIRPasses()"

mehdi_amini accepted this revision.Feb 11 2021, 1:31 PM
This revision is now accepted and ready to land.Feb 11 2021, 1:31 PM
mehdi_amini added inline comments.Feb 11 2021, 1:32 PM
flang/test/Fir/fir-types.fir
2–3

Nit: types ;)

Address review comments

clementval marked 2 inline comments as done.Feb 11 2021, 1:38 PM
clementval added inline comments.
flang/test/Fir/fir-types.fir
2–3

My bad :)

schweitz accepted this revision.Feb 12 2021, 3:17 PM
This revision was automatically updated to reflect the committed changes.
clementval marked an inline comment as done.
clementval reopened this revision.Feb 16 2021, 7:25 AM
This revision is now accepted and ready to land.Feb 16 2021, 7:25 AM
This revision was automatically updated to reflect the committed changes.