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

Build system tweaks #246

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sgligorijevicTT
Copy link
Contributor

@sgligorijevicTT sgligorijevicTT commented Feb 7, 2025

Ticket

Closes #232

What's changed

I made tt-xla pass down the build type to tt-mlir.
I extracted the directives specifying the compilers to use into a script that calls cmake. There was a typo in one of them, and they likely weren't doing anything as they were in the wrong place in the CMakeLists.txt(inspecting the final .so showed mentions of both GCC 9 and Clang 17). This is one of the reasons why hardcoding a compiler in the CMakeLists.txt is considered an anti-pattern. The new configure script is both a correctness and a QoL improvement.
I changed the activate script to set TTMLIR_TOOLCHAIN_DIR to a default value(same one as tt-mlir default) instead of erroring when not set, as a QoL improvement
I added a build convenience script

What hasn't changed

It's still hardcoded to use ccache for tt-mlir irrespective if it's used for tt-xla or not, I ran into build issues when trying to pass down CMAKE_CXX_COMPILER_LAUNCHER to tt-mlir in cases when it isn't set.

Checklist

  • New/Existing tests provide coverage for changes

Copy link

github-actions bot commented Feb 7, 2025

TestsPassed ✅Skipped ⚠️Failed
TT-XLA Tests599 ran436 passed163 skipped0 failed
TestResult
No test annotations available

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.82%. Comparing base (9ee5247) to head (290851d).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #246   +/-   ##
=======================================
  Coverage   77.82%   77.82%           
=======================================
  Files          21       21           
  Lines        1028     1028           
=======================================
  Hits          800      800           
  Misses        228      228           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sgligorijevicTT
Copy link
Contributor Author

As it turns out, there are some good reasons not to build tt-mlir in debug alongside tt-xla

  1. It makes running tests much slower, a test that took 10-ish seconds now takes ~5min. That will be fixed by Skip op verification in opDebugString tt-mlir#2157
  2. The runtime is noisy. Without some sort of filtering, the outputs are close to unusable.

As an alternative, I could add a separate cmake variable to control tt-mlir build type and default it to release, then users that want the extra info can easily set it trough the configure script.

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.

Make tt-mlir inhert the build type
2 participants