[PATCH] D73778: [mlir] Revise naming of MLIROptMain and MLIRMlirOptLib

[Marius Brehler via Phabricator via llvm-commits]

marbre planned changes to this revision.
marbre added inline comments.


================
Comment at: mlir/include/mlir/Support/MlirOpt.h:25
 
-LogicalResult MlirOptMain(llvm::raw_ostream &os,
-                          std::unique_ptr<llvm::MemoryBuffer> buffer,
-                          const PassPipelineCLParser &passPipeline,
-                          bool splitInputFile, bool verifyDiagnostics,
-                          bool verifyPasses);
+LogicalResult MlirOpt(llvm::raw_ostream &os,
+                      std::unique_ptr<llvm::MemoryBuffer> buffer,
----------------
jpienaar wrote:
> 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.
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:

  # possibility (as proposed so far):
    - `MLIROptMain` -> `MLIROpt`
    - `MLIRMlirOptLib` -> `MLIRMlirOpt`
  # 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.


================
Comment at: mlir/tools/mlir-opt/CMakeLists.txt:15
   mlir-opt.cpp
 )
+target_link_libraries(MLIRMlirOpt ${LIB_LIBS})
----------------
mehdi_amini wrote:
> 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)
> 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73778/new/

https://reviews.llvm.org/D73778