[PATCH] D77824: [mlir] Emit errors if global constructors are found within lib/

Sat Apr 11 15:28:34 PDT 2020

bondhugula marked an inline comment as done.
bondhugula added inline comments.

================
Comment at: mlir/lib/IR/CMakeLists.txt:2
+# TODO: Remove the need for global constructors within IR/
+add_flag_if_supported("-Wno-global-constructors" WNO_GLOBAL_CONSTRUCTOR_MLIR_IR)
+
----------------
mehdi_amini wrote:
> bondhugula wrote:
> > mehdi_amini wrote:
> > > bondhugula wrote:
> > > > mehdi_amini wrote:
> > > > > bondhugula wrote:
> > > > > > This should be added to test/lib/ as well?
> > > > > We could, but we don't really have a reason to limit what happens in tests do we?
> > > > > 
> > > > > The motivation for doing this right now is about preserving the ability to build and embed the compiler in a product without having these ctors.
> > > > tests/lib is part of mlir-opt. Doesn't static init affect the latter's load time?
> > > mlir-opt is a testing tool, not a production use-case. Unless there is a point where it measurably hinders developers velocity I'm not sure we should optimize for it.
> > mlir-opt is run roughly 1800 times each time 'check-mlir' is run and nearly all of them as you know are on small inputs. If static init has a noticeable impact on anything built with the libraries, it'll be noticed on check-mlir. We can actually verify it by timing check-mlir before and after this sequence of commits. 
> >  If static init has a noticeable impact on anything built with the libraries, it'll be noticed on check-mlir.
> 
> I doubt it:
> 1) it'll be dominated by everything else, note that a large part of LLVM itself is linked in mlir-opt.
> 2) mlir-opt will still likely run the same amount of registration code, it just gonna do it explicitly instead of relying on the global constructors. To start seeing a difference, I would add `if (argc ==1) return 0` at the beginning of `main()` in mlir-opt so that it becomes a no-op and run it in a loop.
> 
I see - thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77824