Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing CMake deps for THLO and suspect layering #45

Open
stellaraccident opened this issue Aug 15, 2022 · 8 comments
Open

Missing CMake deps for THLO and suspect layering #45

stellaraccident opened this issue Aug 15, 2022 · 8 comments
Assignees

Comments

@stellaraccident
Copy link

Can someone please land an equivalent of this change?

https://github.com/iree-org/iree-mhlo-fork/commit/a6a3308969278cf9ec36132fc9235314fd091c23

I only did a little bit of poking to get a minimum size patch, but the dependency chain and code organization of these pieces seems wrong. Can it be improved? Many of us are not using mlir-hlo as part of XLA where maybe this is tolerated, and it is counter-intuitive that the code is laid out to have such a large transitive dependency set of compiler internals just to get the top level dialect.

@stellaraccident
Copy link
Author

Additional fix needed to satisfy the BFD shared linker: iree-org/iree@61e0e46

@stellaraccident
Copy link
Author

Pushed a review upstream that will make this more strict and less likely to be caught by downstreams: https://reviews.llvm.org/D131911

@stellaraccident
Copy link
Author

stellaraccident commented Aug 15, 2022

Using my super-powers to look at the internal CI, I see that it is:

  1. Not building the Python bindings at all.
  2. Using LLD vs BFD (shouldn't matter with the above upstream patch but BFD tends to be a bit more strict in the absence of further guidance).
  3. Doing a static library build (vs shared, which tends to surface more issues).

I would address 1 by enabling the Python bindings. And for 3, I would change to BUILD_SHARED_LIBS=ON and see what breaks.

I wouldn't do anything about 2 (the above patch to LLVM should make them all strict).

@stellaraccident
Copy link
Author

Upstream patch landed. If you now enable the Python bindings in your CI at that patch or later, I expect that you will get a failure equivalent to what we experienced downstream.

@stellaraccident
Copy link
Author

(and this will also make sure it works on Windows)

@stellaraccident
Copy link
Author

(separately, we would also like the project layered better so that it is possible to depend on MHLO and specific conversions/passes vs the transitive "everything")

@burmako
Copy link
Contributor

burmako commented Aug 15, 2022

Stella, thank you very much for filing the bug report and for digging into the details! We'll get this handled.

copybara-service bot pushed a commit that referenced this issue Aug 16, 2022
Fixes the immediate problem behind #45 using patches by Stella.

This CL doesn't aim to address the layering part of the issue, e.g. MHLO passes depending on the THLO dialect. For now, it just fixed the blocker on the IREE side where a symbol in a shared library (the THLO type descriptor) is failing to resolve at runtime.

PiperOrigin-RevId: 467966552
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this issue Aug 16, 2022
Fixes the immediate problem behind tensorflow/mlir-hlo#45 using patches by Stella.

This CL doesn't aim to address the layering part of the issue, e.g. MHLO passes depending on the THLO dialect. For now, it just fixed the blocker on the IREE side where a symbol in a shared library (the THLO type descriptor) is failing to resolve at runtime.

PiperOrigin-RevId: 467966552
@sherhut
Copy link

sherhut commented Aug 17, 2022

Thanks for the report and patch. As you noted, we did not see this issue as it does not show up on our CI.

We are not using the cmake build files ourselves and did not notice the layering violation. I have split out the thlo lowerings into their own target in commit 21a5d7d. I hope this fixed your immediate layering concerns.

As to the dependency of thlo on gml_st: We will move interfaces into their own targets eventually. We have forked the upstream TlingInterface to prototype a different version before bringing it up in an RFC, so the current layering in cmake is temporary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants