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

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

Kayjukh accepted this revision.
Kayjukh marked 7 inline comments as done.
Kayjukh added inline comments.


================
Comment at: mlir/examples/standalone/standalone-opt/standalone-opt.cpp:25
+using namespace llvm;
+using namespace mlir;
+
----------------
marbre wrote:
> If it is an out-of-tree example, I would avoid `using namespace`.
Is there any particular reason for this? IMO the two `using namespace` directives make the code easier to read by removing a lot of syntactic overhead. I can understand that in a larger project there might be a potential issue with names colliding but if this is the case, then the maintainer of said project can remove those lines and adapt the code as needed.


================
Comment at: mlir/examples/standalone/standalone-opt/standalone-opt.cpp:58
+  registerAllDialects();
+  registerAllPasses();
+
----------------
marbre wrote:
> One could avoid registering all core dialects and passes here, but it might be more useful to change this in a follow-up.
Yes, agreed, a follow-up may be a better place for this. For now my goal was to have the same behavior as the one described in the documentation when adding your own in-tree dialect to `mlir-opt`.


================
Comment at: mlir/examples/standalone/standalone-opt/standalone-opt.cpp:61
+  registerDialect<standalone::StandaloneDialect>();
+  // TODO: Register standalone passes here.
+
----------------
marbre wrote:
> A standalone pass is out of scope of this example?
I was thinking about adding one in a potential follow-up to this patch. As mentioned in the description, this is a bare bones example that could largely be expanded upon. But I think it makes more sense to add additional features one at a time so as not to make a huge patch with everything one would think of in one go. Would this approach make more sense?


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

https://reviews.llvm.org/D77133