[PATCH] D77133: [mlir] Add an out-of-tree dialect example

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

This LGTM other than inline comments, you likely should try to wait for River's approval as well.



================
Comment at: mlir/examples/standalone/README.md:10
+mkdir build && cd build
+cmake -G Ninja .. -DCMAKE_LINKER=<path-to-linker> -DMLIR_DIR=$PREFIX/lib/cmake/mlir -DLLVM_EXTERNAL_LIT=$BUILD_DIR/bin/llvm-lit
+cmake --build . --target check-standalone
----------------
Why is `-DCMAKE_LINKER=` needed here?


================
Comment at: mlir/examples/standalone/README.md:17
+```
+**Note**: Make sure to pass `-DLLVM_INSTALL_UTILS=ON` when building LLVM with CMake in order to install `FileCheck` to the chosen installation prefix.
+
----------------
This is beyond this revision, but I think adding an option for having an external FileCheck just like for `-DLLVM_EXTERNAL_LIT` would help here.


================
Comment at: mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp:22
+  // Parse the custom assembly format.
+  if (parser.parseOperand(operand) ||
+      parser.parseOptionalAttrDict(result.attributes) ||
----------------
rriddle wrote:
> Kayjukh wrote:
> > rriddle wrote:
> > > Should you be using the declarative assembly format instead?
> > > 
> > > https://mlir.llvm.org/docs/OpDefinitions/#declarative-assembly-format
> > I considered using the declarative syntax at first, but I think using a custom parser and printer illustrates a more general (albeit rare) case where you might want to do more during parsing than what is done by the generated parser.
> > 
> > But as I said in a previous update comment, I am very much open to discussion on this point. I don't know if it is relevant to account for such corner cases in an example dialect.
> It all depends on what you are trying to convey with your example. Given that examples are generally used as a template, I would rather do the right thing instead. For example, you could have defined the operation classes manually in C++ but you didn't. Why shouldn't the same apply to the assembly format?
+1 with River here. If this is a template I would go with the most idiomatic way (otherwise you could just not use ODS at all).



================
Comment at: mlir/examples/standalone/standalone-opt/CMakeLists.txt:32
+        LLVMCore
+        LLVMAsmParser
+
----------------
The list seems much much too long here, why do you need LLVMAsmParser for instance?
I'm concerned about how this is unmaintainable.



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

https://reviews.llvm.org/D77133