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

Prevent ttlmir-opt/translate from using llvm.so #1525

Merged
merged 8 commits into from
Dec 9, 2024
Merged

Conversation

vwellsTT
Copy link
Contributor

@vwellsTT vwellsTT commented Dec 6, 2024

Found that both ttmlir-opt and ttmlir-translate are still linking to LLVM dylib, despite DISABLE_LLVM_LINK_LLVM_DYLIB flags in respective CMakeLists.txt. Changing this flag seems to remedy the problem.

@vwellsTT vwellsTT marked this pull request as ready for review December 6, 2024 19:17
Copy link
Contributor

@nsmithtt nsmithtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suspiciously no fallout 🧐, we'll take it if it's true! Though I'm not sure the GH action actually tested it.

@vmilosevic, I don't think this triggered a docker rebuild just looking through the GH actions.

@vwellsTT
Copy link
Contributor Author

vwellsTT commented Dec 8, 2024

Suspiciously no fallout 🧐, we'll take it if it's true! Though I'm not sure the GH action actually tested it.

Yeah, not sure I trust that either haha. Are there comprehensive manual tests I can run, if there isn't convenient way to force CI to pick up changes?

@vmilosevic
Copy link
Contributor

The image tag you are looking for (ghcr.io/tenstorrent/tt-mlir/tt-mlir-ci-ubuntu-22-04:dt-dcb58a0a097d08b1e54cc8eb4e48ec854b4217bcefb8ae2f39e3280af6288a7f) was already built by previous run on same branch https://github.com/tenstorrent/tt-mlir/actions/runs/12200962107/job/34038297242
And it was again built in a run after that https://github.com/tenstorrent/tt-mlir/actions/runs/12202629226/job/34043759728
So it was already prepared when the last run was executed.
I'm not sure why it was built twice like it was deleted from GHCR between runs.
If you want to verify linking in CI we can print linker info from CI script, something like

ldd tt-mlir | grep llvm.so

@vwellsTT
Copy link
Contributor Author

vwellsTT commented Dec 9, 2024

Does this mean the CI tests did indeed run on this new env? I've been using this change on my own branch to force ttmlir-translate/opt to not link to .so, so I'm fairly confident that part works--more worried it will also break something that actually needs to link to .so

@nsmithtt
Copy link
Contributor

nsmithtt commented Dec 9, 2024

The image tag you are looking for (ghcr.io/tenstorrent/tt-mlir/tt-mlir-ci-ubuntu-22-04:dt-dcb58a0a097d08b1e54cc8eb4e48ec854b4217bcefb8ae2f39e3280af6288a7f) was already built by previous run on same branch https://github.com/tenstorrent/tt-mlir/actions/runs/12200962107/job/34038297242 And it was again built in a run after that https://github.com/tenstorrent/tt-mlir/actions/runs/12202629226/job/34043759728 So it was already prepared when the last run was executed. I'm not sure why it was built twice like it was deleted from GHCR between runs. If you want to verify linking in CI we can print linker info from CI script, something like

ldd tt-mlir | grep llvm.so

I see, OK, then I think we should be safe to land it then. We'll keep a close eye on it.

@vwellsTT vwellsTT enabled auto-merge (squash) December 9, 2024 17:55
@vwellsTT vwellsTT merged commit 8abdc36 into main Dec 9, 2024
20 checks passed
@vwellsTT vwellsTT deleted the vwells/llvm_dylib branch December 9, 2024 23:26
azecevicTT pushed a commit that referenced this pull request Dec 17, 2024
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

Successfully merging this pull request may close these issues.

3 participants