Page MenuHomePhabricator

Keep output file after successful execution of mlir-opt
ClosedPublic

Authored by LukasSommerTu on Apr 7 2020, 5:26 AM.

Details

Summary

Invoke keep() on the output file of mlir-opt in case the invocation of MlirOptMain was successful, to make sure the output file is not deleted on exit from mlir-opt.
Fixes a similar problem in standalone-opt from the example for an out-of-tree, standalone MLIR dialect.

This revision also adds a missing parameter to the invocation of MlirOptMain in standalone-opt.

Diff Detail

Event Timeline

LukasSommerTu created this revision.Apr 7 2020, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 5:26 AM
ftynse added a subscriber: ftynse.Apr 7 2020, 5:59 AM

Can we rather do it inside MlirOptMain instead of repeating the code?

In the current design not. MlirOptMain does not receive the file itself, but rather an output stream, so it cannot manage the file.

We could of course change the signature of MlirOptMain to accept a ToolOutputFile instead, but that would loose some generality, because one would always have to create a ToolOutputFile for invoking MlirOptMain instead of the more general raw_ostream. Alternatively, we could add another method to MlirOptMain.cpp that manages the file and internally invokes the original MlirOptMain again.

What would you prefer?

Kayjukh added a subscriber: Kayjukh.Apr 7 2020, 8:56 AM
Kayjukh added inline comments.Apr 7 2020, 9:07 AM
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
49

Should this be part of a separate patch? It is not really related to your proposed change.

I think it would make sense to propagate the changes to mlir-opt as well.

I think it would make sense to propagate the changes to mlir-opt as well.

The changes to mlir-opt.cpp propagate the change of the file-management to mlir-opt. Or did you refer to something else?

mlir/examples/standalone/standalone-opt/standalone-opt.cpp
49

I can make a separate patch for that, but then we would need to pass a magic constant true or false to MlirOptMain, to match the number of required parameters.

But if you prefer to have that in a separate patch, I can extract it. What do you prefer?

mehdi_amini accepted this revision.Apr 7 2020, 11:16 AM
This revision is now accepted and ready to land.Apr 7 2020, 11:16 AM
Kayjukh added inline comments.Apr 7 2020, 11:46 AM
mlir/examples/standalone/standalone-opt/standalone-opt.cpp
49

I was thinking about making a patch for this command line argument first, and then adding the call to keep. At least I would update the title and description to mention these changes.

I think it would make sense to propagate the changes to mlir-opt as well.

The changes to mlir-opt.cpp propagate the change of the file-management to mlir-opt. Or did you refer to something else?

Nvm, for some reason I overlooked the changes to mlir-opt.cpp.

LukasSommerTu edited the summary of this revision. (Show Details)Apr 7 2020, 12:05 PM
LukasSommerTu marked 4 inline comments as done.Apr 7 2020, 12:08 PM

@mehdi_amini: I do not have commit access, could you please commit the revision for me?

mlir/examples/standalone/standalone-opt/standalone-opt.cpp
49

I've updated the description to state the changes to standalone-opt more clearly.

This revision was automatically updated to reflect the committed changes.