LGTM

I think the MLIR parts of this code were written before attempting to create libMLIR.so. Probably these tablegen binaries would have to also avoid linking against libMLIR.so and be statically linked to both llvmSupport and MLIRsupport to work properly. I think the proposed solution is reasonable.

LGTM... Does LLVM_TOOLS_BINARY_DIR include CMAKE_CFG_INTDIR? Is this actually NFC?

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

In D118148#3269641, @stellaraccident wrote:

There have been so many permutations of this that I can't remember my expectation for issues here, but I feel like this was causing other issues. Have you tried this on a manylinux image? Iirc, that was where things went bad when module detection didn't work. (I can give this a shot if you haven't)

Thanks!

Thanks for this!

LGTM

LGTM

Seems like a reasonable refactoring to me.

LGTM

Rolled into D113732

Thanks!

I think this can go in tree now.

Have you tried this on any graph regions? I guess maybe the same question goes for ForwardDataflowAnalysis?

Thanks, Morten!

In D108098#2945907, @stellaraccident wrote:

Read my mind - was meaning to do this very change last week but ran out of time. How much further are you going? I'd ultimately like our targets to all have proper include/link directories defined so we don't need to do so much global munging.

Yeah, still broken.

I think this is obsolete?

Thanks for the reviews!

This seems reasonable to me.

Generally speaking I like this approach... It's much simpler than the alternatives that I've seen. I think we should try to take it to the next step....

if not installing into python/ anymore, can you document where things will get installed? I didn't quite parse it out of the code.

It looks like this change to create symlinks in the build area also results in those symlinks getting installed in the install area... This seems like a bad idea, since the install area ends up having symlinks back to the source tree. However, I don't see a simple way to change the install action to fix that. Any ideas here?

In D102740#2791585, @craig.topper wrote:

Seems fine, but is there a way to write this in tablegen that would trigger the assert?

Ping... in general is there an issue converting asserts() to proper error messages in tablegen?

Thanks for looking at this!