[PATCH] D74461: Remove static registration for dialects, and the "alwayslink" hack for passes

[Stephen Neuendorffer via Phabricator via llvm-commits]

stephenneuendorffer added a comment.

I think getting rid of whole_archive_link is a good thing, but I'm concerned that this actually makes it somewhat more complicated to get new Passes and Dialects into the system in a not error-prone way.



================
Comment at: mlir/examples/toy/Ch7/toyc.cpp:23
 #include "mlir/IR/Module.h"
+#include "mlir/InitAllDialects.h"
 #include "mlir/Parser.h"
----------------
Why "Init" vs. "register"? A little bikeshedding, but when it's this close, it's really annoying that it's not exactly the same.. 


================
Comment at: mlir/include/mlir/InitAllPasses.h:18-30
+#include "mlir/Conversion/GPUToCUDA/GPUToCUDAPass.h"
+#include "mlir/Conversion/GPUToNVVM/GPUToNVVMPass.h"
+#include "mlir/Conversion/GPUToROCDL/GPUToROCDLPass.h"
+#include "mlir/Conversion/GPUToSPIRV/ConvertGPUToSPIRVPass.h"
+#include "mlir/Conversion/LinalgToLLVM/LinalgToLLVM.h"
+#include "mlir/Conversion/LinalgToSPIRV/LinalgToSPIRVPass.h"
+#include "mlir/Conversion/LoopsToGPU/LoopsToGPUPass.h"
----------------
Most passes are associated with a Dialect, or the presence of two related Dialects.  Perhaps this could be used to simplify this registration?


================
Comment at: mlir/include/mlir/InitAllPasses.h:56
+
+  // Init general passes
+  createCanonicalizerPass();
----------------
It seems like there should be some hierarchy here.  I don't think it scales to list all the passes here.  At least the ones from a dialect should be 'registered' as part of the dialect registration?
Ultimately, getting rid of whole-program-link is good, but is it really better to convert it to a list of passes here?


================
Comment at: mlir/test/EDSC/builder-api-test.cpp:40
 static MLIRContext &globalContext() {
+  static bool init_once = []() {
+    registerDialect<AffineOpsDialect>();
----------------
maybe registerDialects could be a variadic template class, avoiding the need for the functor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74461