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

Add support for stablehlo from tt-mlir project. #447

Merged
merged 17 commits into from
Aug 21, 2024
Merged

Conversation

uazizTT
Copy link
Contributor

@uazizTT uazizTT commented Aug 20, 2024

Add support for stablehlo from tt-mlir project as an embedded project.

Stablehlo is disabled by default and can be enabled by turning on TTMLIR_ENABLE_STABLEHLO flag.

@uazizTT uazizTT changed the title Mrakita/stablehlo Add support for stablehlo from tt-mlir project. Aug 20, 2024
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.

Usman this looks great, thank you, I think it's looking pretty clean.

Can we add it CI? Enabled on ubuntu, disabled on Mac, that way we kind of get coverage of both build paths?

CMakeLists.txt Outdated Show resolved Hide resolved
@nsmithtt
Copy link
Contributor

nsmithtt commented Aug 20, 2024

You have some failing tests, because it's disabled in build:

FAIL: TTMLIR :: ttmlir/Dialect/TTIR/stablehlo_to_ttir_addop.mlir (24 of 44)

See test/lit.site.cfg.py.in, as an example see config.enable_bindings_python, but we probably want something like:

config.enable_stablehlo = @TTMLIR_ENABLE_STABLEHLO@ and "@TTMLIR_ENABLE_STABLEHLO@" == "ON"

And then inside of test/lit.cfg.py:

if config.enable_stablehlo:
    config.available_features.add("stablehlo")

Finally, inside of the test:

// REQUIRES: stablehlo

See https://llvm.org/docs/TestingGuide.html for more info.

@uazizTT uazizTT requested a review from vmilosevic as a code owner August 20, 2024 02:52
@uazizTT uazizTT force-pushed the mrakita/stablehlo branch from 8c9e97f to 58243b7 Compare August 20, 2024 02:56
@nsmithtt
Copy link
Contributor

nsmithtt commented Aug 20, 2024

Now failing with:

  add_subdirectory given source "/opt/ttmlir-toolchain/src/stablehlo" which
  is not an existing directory.

@vmilosevic will the environment build automatically kick off a new docker build? This used to use GitHub cache

This commit updates the env

@vmilosevic
Copy link
Contributor

vmilosevic commented Aug 20, 2024

Now failing with:

  add_subdirectory given source "/opt/ttmlir-toolchain/src/stablehlo" which
  is not an existing directory.

@vmilosevic will the environment build automatically kick off a new docker build? This used to use GitHub cache

This commit updates the env

No, it's not automated, you need to manually gick off the docker image build.

image

I added a change to build docker images on self-hosted runner, building large images on Github runners sometimes runs out of space and also fails randomly without any log :(

Build job:
https://github.com/tenstorrent/tt-mlir/actions/runs/10468325323

@vmilosevic
Copy link
Contributor

My bad, the docker build was always using code from the main, not the current branch 🤦‍♂️
I'm adding a fix now and building the image again.

@uazizTT uazizTT force-pushed the mrakita/stablehlo branch from 1b829a6 to a24b381 Compare August 20, 2024 19:15
@uazizTT uazizTT force-pushed the mrakita/stablehlo branch from a24b381 to 57694f1 Compare August 20, 2024 23:01
@uazizTT uazizTT requested a review from nsmithtt August 21, 2024 02:40
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.

Changes look good on my end, let's get sign off from @vmilosevic on the docker file changes

@vmilosevic
Copy link
Contributor

👍 All is good, Docker changes are my commits

@uazizTT uazizTT merged commit d33cd6a into main Aug 21, 2024
8 of 9 checks passed
@uazizTT uazizTT deleted the mrakita/stablehlo branch August 21, 2024 11:20
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.

4 participants