Fixed in https://reviews.llvm.org/rGaa78c5298ea37f2ca8150dc0a1c880be7ec438f4

FYI - this patch has a minor bug in it when used as part of larger projects. Line 214 creates duplicate target names in some cases. It needs to be scoped (like is done for the target just below it).

In D128593#3615157, @mehdi_amini wrote:

I have some concerns with:

Define a register_dialects(registry: DialectRegistry) function that can extend the DialectRegistry that will be used to bootstrap the Context.
Define a context_init_hook(context: Context) function that will be added to a list of callbacks which will be invoked after dialect registration during Context initialization.

Because contrary to pass registration for example, there isn't a global context or global dialect registry in MLIR: that means that I should be able to create a context inside my python library and not have it "polluted" by something that was registered by another library for its own purpose.

That said this is still quite an big step forward from where we're at right now, even though I fear we may dig ourselves in a place where it'll be hard to get out of.

In D128593#3614871, @ftynse wrote:

I am not a huge fan of magic method / file names, but this is a common thing to do in Python. My only usability concern is about debugging:

with Context():
  ir.parse("custom.func @foo()",)

will work or break depending on where the Context is imported from: from mlir.ir import Context vs from custom.ir import Context. I suppose we can get used to it.

A few very minor nits, but otherwise lgtm.

Comments.

Move closer to end state.

In D128593#3611605, @ftynse wrote:

Would you mind posting a PSA on the forum copying the change description and the tentative evolution plan?

I think we need to revisit this patch as it is broken for some very common cases. Left notes here: https://github.com/llvm/llvm-project/issues/56235

Fix fir test.

In D128593#3611022, @stellaraccident wrote:

In D128593#3611008, @tschuett wrote:

Do you believe that there is a place for a HowTo.md that makes downstream users really happy?

As in a "How to Integrate into a Downstream Project?" guide. Yes, we should create that.

In D128593#3611008, @tschuett wrote:

Do you believe that there is a place for a HowTo.md that makes downstream users really happy?

Rebase

In D128230#3608565, @stella.stamenova wrote:

In D128230#3606600, @stellaraccident wrote:

Thank you for this. As we discussed on Discord, I didn't know about the inheritable sources on interface libraries, and that simplifies things. A couple of comments for my own education, but otherwise looks good.

Have you happened to have tested this with a downstream yet?

I made sure torch-mlir can build/test/install correctly both with a build and an install tree of mlir. I haven't tested other downstreams.

Thank you for this. As we discussed on Discord, I didn't know about the inheritable sources on interface libraries, and that simplifies things. A couple of comments for my own education, but otherwise looks good.

(also sorry for some auto correct typos - on mobile)

Thanks for this. Just one comment of the variety of "have you considered doing it entirely differently) :) I'm not convinced either way -- open to discuss

Nice! Are the builtin dialect docs auto-generated somehow, or is there something we should update in this patch? (https://mlir.llvm.org/docs/Dialects/Builtin/#arrayattr)

Thanks for the contribution. I'll land it at my next opportunity, then.

Thanks - good catch. Do you have commit access to land?

Thanks for doing this!

Idk - I haven't seen the failures. It passed presubmit Benoit I landed (phab says it didn't because I did a trivial rebase/addressed comments right before).

In D126585#3544069, @jpienaar wrote:

Nice. Would integration go to heavy then or still be separate? (I was even wondering if this test should be considered an integration test, and indeed this one is quite slow while not providing much signal, but we do want to make sure this flow isn't broken)

comments and rebase

Remove braces.

In D126230#3550833, @jpienaar wrote:

Looks good thanks!

Tweak based on comments.

Bazel updates.

Comments.

Comments

In D126446#3540627, @rriddle wrote:

In D126446#3539427, @mehdi_amini wrote:

What about using directly YAML for this section?

I considered, but ultimately it felt like more complexity than the benefits it brought (especially for this draft). Having something
just for us felt much simpler and more expressive (we could use Attributes/Types for config values if we wanted). I could likely be
convinced otherwise though, using something like YAML would conceptually make supporting more complex config values
easier (nested structs and stuff) if we wanted to go that approach. There are tradeoffs though (e.g. error reporting, attribute/type values,
still need a generic interface to enable seamless bitcode integration, etc.).

Generally LGTM, although we may want to further constrain this in a follow-up. In general, I am pro having optimization passes and having them be truly optional. I would be tempted to name this "TensorDataConstantOptimizationPass".

Add chains.

Comments

I think Mehdi wrote some of this originally. I'll review as well but need to study it...

In D126230#3535583, @Mogball wrote:

*breathes a sigh of relief that no ODS bugs were stepped on*

Note: design discussion continuing on discourse: https://discourse.llvm.org/t/rfc-introduce-ml-program-dialect-and-top-level-ops-proposal-v2/60907/51

Yes, I will land it either tomorrow or this weekend. Thank you for the patch!

Comments

Comments

Update bazel files.

Update bazel files.

Jacques/Jeff, please take a look if you have a chance. I see there is a patch pending to fix the attribute parser declarations, but that will be a minor adaptation. I think the IR format is good enough as I have it here.

Rebase and adapt to self-type attribute fix.

In D124318#3499605, @Mogball wrote:

Yeah that's... unfortunate to hear. ODS can be a bag of knives sometimes. I'll take a look at the bugs.

Thanks!

Thank you for the clarifications. lgtm.

In D125182#3505196, @stephenneuendorffer wrote:

Seems reasonable. If this works it would be nice to just hand it off to the cmake folks...

In D125284#3502486, @stella.stamenova wrote:

In D125284#3502410, @stellaraccident wrote:

I am glad that an actual Windows dev (versus someone who just occasionally plays one on TV) is looking at this. I've never understood the windows debug/release/runtime version dichotomies when not just working on a stock VS based project that automatically does the right thing.

I'm fine with landing if we special case the header like I suggest. Otherwise, open to discuss more.

To be honest, I don't think anyone understands this. It took me half a day of looking at source, building various bits and then looking at python, cmake and pybind11 sources to figure it out. I only hope I've documented enough of it so that next time we can skip most of it :)

I am glad that an actual Windows dev (versus someone who just occasionally plays one on TV) is looking at this. I've never understood the windows debug/release/runtime version dichotomies when not just working on a stock VS based project that automatically does the right thing.

In D125182#3500550, @mehdi_amini wrote:

Does this print how to run the python file in case of CMake failure?