Page MenuHomePhabricator

[mlir] Revise naming of MLIROptMain and MLIRMlirOptLib
ClosedPublic

Authored by marbre on Fri, Jan 31, 5:10 AM.

Details

Summary
  • Rename CMake target MLIROptMain to MLIROptLib: The target provides the main library
  • Rename CMake target MLIRMlirOptLib to MLIRMlirOptMain: The target provides the main() entry function

At the moment, the Bazel configuration of TenorFlow maps the target MlirOptLib to "lib/Support/MlirOptMain.cpp" and MlirOptMain to "tools/mlir-opt/mlir-opt.cpp". This is the other way around in the CMake configuration. As discussed in the context of the pull request https://github.com/tensorflow/tensorflow/pull/36301, it seems useful to revise the naming in the MLIR repo.

Diff Detail

Event Timeline

marbre created this revision.Fri, Jan 31, 5:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jan 31, 5:10 AM

Unit tests: pass. 62365 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jpienaar added inline comments.Fri, Jan 31, 8:14 AM
mlir/include/mlir/Support/MlirOpt.h
1 ↗(On Diff #241701)

s/main/entry point/ (especially if we remove other main references).

25 ↗(On Diff #241701)

This was chosen this way to match TableGenMain entry point specification.

My preference would be to just update the CMake files without renaming the file or function: this feels stylistic with no real increase in clarity but would require updating all callers in dependent project. That way you get the consistency between cmake and bazel without further changes.

nicolasvasilache resigned from this revision.Fri, Jan 31, 11:56 AM
mehdi_amini added inline comments.Fri, Jan 31, 12:05 PM
mlir/tools/mlir-opt/CMakeLists.txt
16

Why is this a library? It seems not used right now.

(I see it is passed in the link of mlir-opt below, but that seems like a no-op since the only source file for the library is also passed directly as an object to this target)

marbre planned changes to this revision.Sat, Feb 1, 4:32 AM
marbre added inline comments.
mlir/include/mlir/Support/MlirOpt.h
25 ↗(On Diff #241701)

Thanks for your review. Prior to sending a new diff (which excludes renaming of files and the function), what target names would you propose? Just swapping MLIROptMain and MLIRMlirOptLib seems to break the naming convention. I see the following possibilities:

  1. possibility (as proposed so far):
    • MLIROptMain -> MLIROpt
    • MLIRMlirOptLib -> MLIRMlirOpt
  2. possibility
    • MLIROptMain -> MLIROptLib
    • MLIRMlirOptLib -> MLIRMlirOptMain

The second would match TensorFlow's Bazel configuration closest, but to me it doesn't feel quite so clear. Looking forward for your feedback.

mlir/tools/mlir-opt/CMakeLists.txt
16

I think the dep of mlir-opt on MLIROptMain could be dropped. So having MLIROptMain as a library would not be necessary here, but it's nice for out-of-tree projects, like IREE.

marbre updated this revision to Diff 242341.Tue, Feb 4, 8:05 AM
marbre edited the summary of this revision. (Show Details)

I went for the second option to get close to TensorFlow's Bazel configuration.

jpienaar accepted this revision.Tue, Feb 4, 3:37 PM

This gets us closer to matching the bazel side and works for me, thanks

This revision is now accepted and ready to land.Tue, Feb 4, 3:37 PM

Thanks for the review. I would appreciate if someone could commit the patch on my behalf.

Can you rebase? The patch does not apply

marbre updated this revision to Diff 242817.Thu, Feb 6, 12:16 AM
marbre edited the summary of this revision. (Show Details)

Rebased onto 6bfc45cf6. The patch should apply now.

Gentle ping. I would be happy if someone could commit the patch on my behalf.

This revision was automatically updated to reflect the committed changes.