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

[Jean-Michel Gorius via Phabricator via llvm-commits]

Kayjukh accepted this revision.
Kayjukh added inline comments.


================
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
----------------
mehdi_amini wrote:
> Why is `-DCMAKE_LINKER=` needed here?
It isn't. This was an artifact from one of my previous tests.


================
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.
+
----------------
mehdi_amini wrote:
> 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.
Yes, this would definitely help.


================
Comment at: mlir/examples/standalone/lib/Standalone/StandaloneOps.cpp:22
+  // Parse the custom assembly format.
+  if (parser.parseOperand(operand) ||
+      parser.parseOptionalAttrDict(result.attributes) ||
----------------
mehdi_amini wrote:
> 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).
> 
Ok, yes, makes sense.


================
Comment at: mlir/examples/standalone/standalone-opt/CMakeLists.txt:32
+        LLVMCore
+        LLVMAsmParser
+
----------------
mehdi_amini wrote:
> The list seems much much too long here, why do you need LLVMAsmParser for instance?
> I'm concerned about how this is unmaintainable.
> 
This was originally a simple copy-and-paste from the `mlir-opt` `CMakeLists.txt` file, which for some reason links against all of these libraries.

I updated the list to include the minimum amount of libraries so as not to produce a linker error and still make the tests pass. The user of the template will then be free to link to whatever additional libraries may be needed.


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

https://reviews.llvm.org/D77133