Add dependencies for onnx-mlir to tt-mlir project linked aga… #261

uazizTT · 2024-07-31T16:42:55Z

…inst a specific version of llvm

nsmithtt · 2024-07-31T17:08:14Z

env/build-requirements.txt

@@ -6,3 +6,4 @@ wheel
 setuptools
 black
 pre-commit
+onnx


Do we need this?

onnx is a dependency for onnx-mlir project

It's not listed as a dependency in their project requirements. They really require python to run their optimizer driver?

This is something I found by building it that it requires onnx at least for Mac OSX build, I suspect they forgot to mention in the project requirements.

nsmithtt · 2024-07-31T17:12:03Z

env/CMakeLists.txt

+    GIT_TAG ${ONNX_MLIR_VERSION}
+    GIT_SUBMODULES_RECURSE true
+    DEPENDS python-venv
+)


This ExternalProject_Add should be moved to the third_party subdirectory. I think we should:

git mv third_party/CMakeLists.txt third_party/tt-metal.cmake

You should create third_party/onnx-mlir.cmake along with a new CMakeLists.txt:

include(tt-metal) if (TTMLIR_ENABLE_ONNX_MLIR) include(onnx-mlir) endif

Ok got it, let me update with the changes.

nsmithtt

@uazizTT, this is awesome! A few requests inline. The env and third_party directories are a bit confusing, but generally I split it up this way:

env: Hard dependencies of tt-mlir, infrequently update. Should be thought of as the toolchain needed to do any development with tt-mlir.
third_party: Soft dependencies that add functionality to tt-mlir, potentially frequently updated.

Another reason, is that the idea for env is that it could be cached (and it is in GitHub CI). If any file changes under env/ then the hash changes, so we really want to be deliberate in adding / updating files in this area because llvm is relatively expensive to rebuild.

uazizTT · 2024-07-31T17:32:31Z

Ok thanks for the breakdown. I don't think onnx-mlir will be updated frequently but yes we can categorize it as a soft dependency. Let me move it to third-party directory.

nsmithtt · 2024-07-31T17:38:36Z

env/CMakeLists.txt

+    -DCMAKE_CXX_COMPILER=/usr/bin/c++
+    -DPython3_ROOT_DIR=$pythonLocation
+    -DMLIR_DIR=${TTMLIR_TOOLCHAIN_DIR}/llvm-project/build/lib/cmake/mlir
+    -DCMAKE_INSTALL_PREFIX=${TTMLIR_TOOLCHAIN_DIR}


Can we also add -DONNX_MLIR_BUILD_TESTS=OFF -DONNX_MLIR_ENABLE_JAVA=OFF?

Since build tests are disable, so package onnx is not required anymore, so I removed it from dependencies.

uazizTT

Updated with requested changes.

nsmithtt · 2024-07-31T20:09:53Z

env/build-requirements.txt

@@ -6,3 +6,4 @@ wheel
 setuptools
 black
 pre-commit
+onnx


It's not listed as a dependency in their project requirements. They really require python to run their optimizer driver?

nsmithtt · 2024-07-31T20:11:17Z

third_party/tt-metal.cmake

Perhaps it's just the way that GitHub renders the PR, but this should be a git mv third_party/CMakeLists.txt third_party/tt-metal.cmake so that the blame and history remains intact.

In this case, I am moving content from CMakeLists to tt-metal.cmake and at the same time adding new content to CMakeLists, so that's why it is not recognizing it as a move.

nsmithtt · 2024-07-31T20:13:59Z

third_party/CMakeLists.txt

@@ -1,73 +1,6 @@
-include(ExternalProject)
+option(TTMLIR_ENABLE_ONNX_MLIR "Enable ONNX MLIR support" OFF)


We should move this option to the top level CMakeLists.txt and also enable it in the "Configure CMake" step of .github/workflows/build.yml so that CI can test onnx conversion.

Sure I can move it to CMakeLists.txt. I will add it to build.yml in order to guard against any future changes to LLVM versions that might break the onnx-mlir build.

uazizTT

Added the changes

uazizTT

Added the requested changes

rpavlovicTT · 2024-08-01T07:53:59Z

Nice changes, please fix the PR title and description. Same goes for the first commit. Here is a guide I usually follow.

gfengTT · 2024-08-02T00:07:15Z

third_party/onnx-mlir.cmake

+    PREFIX ${TTMLIR_TOOLCHAIN_DIR}
+    CMAKE_GENERATOR Ninja
+    CMAKE_ARGS
+    -DCMAKE_CXX_COMPILER=/usr/bin/c++


Should this be ${CMAKE_CXX_COMPILER}?

Per the onnx build docs they suggest using this compiler on macOS, I think it's just clang, but in some kind of compatibility mode. That said we should see test we really need it, might be sufficient to use regular clang.

nsmithtt · 2024-08-02T01:36:29Z

third_party/onnx-mlir.cmake

+    CMAKE_GENERATOR Ninja
+    CMAKE_ARGS
+    -DCMAKE_CXX_COMPILER=/usr/bin/c++
+    -DPython3_ROOT_DIR=$pythonLocation


I'm not sure this line is legal cmake syntax. I think $pythonLocation refers to some environment variable the suggest setting in their build docs, but they were running a bash command, I think this line gets evaluated by cmake before it constructs the underlying shell string.

nsmithtt · 2024-08-02T01:37:20Z

third_party/tt-metal.cmake

Nit, this should be a git mv

I did use 'git mv' for this but it doesn't show as such, probably because I am not just moving content from CMakeLists to tt-metal.cmake but also adding new content to CMakeLists at the same time.

…tor third_party build to accomodate multiple soft dependencies.

…t-to-tt-mlir

nsmithtt · 2024-08-03T14:27:36Z

env/CMakeLists.txt

@@ -45,6 +45,7 @@ ExternalProject_Add(
    -DPython3_EXECUTABLE=${TTMLIR_TOOLCHAIN_DIR}/venv/bin/python
    -DCMAKE_INSTALL_PREFIX=${TTMLIR_TOOLCHAIN_DIR}
    -DLLVM_ENABLE_PROJECTS=mlir
+    -DLLVM_ENABLE_RTTI=ON


Ooof, this sucks :(

Ok this is actually in deal breaker territory, let's meet next week to discuss our options. I'll also file an issue on onnx-mlir to see if they ever plan on removing this requirement: onnx/onnx-mlir#2897

uazizTT requested a review from nsmithtt as a code owner July 31, 2024 16:42

nsmithtt reviewed Jul 31, 2024

View reviewed changes

nsmithtt requested changes Jul 31, 2024

View reviewed changes

nsmithtt reviewed Jul 31, 2024

View reviewed changes

uazizTT assigned nsmithtt Jul 31, 2024

uazizTT commented Jul 31, 2024

View reviewed changes

nsmithtt requested changes Jul 31, 2024

View reviewed changes

uazizTT requested a review from vmilosevic as a code owner July 31, 2024 22:11

uazizTT requested a review from AleksKnezevic July 31, 2024 22:11

uazizTT commented Jul 31, 2024

View reviewed changes

uazizTT closed this Jul 31, 2024

uazizTT reopened this Jul 31, 2024

uazizTT unassigned nsmithtt Aug 1, 2024

gfengTT reviewed Aug 2, 2024

View reviewed changes

uazizTT force-pushed the integrate-onnx-mlir-project-to-tt-mlir branch from 96a7c41 to 2e7b420 Compare August 2, 2024 01:32

nsmithtt reviewed Aug 2, 2024

View reviewed changes

uazizTT force-pushed the integrate-onnx-mlir-project-to-tt-mlir branch from 2e7b420 to c5d652b Compare August 2, 2024 01:47

uazizTT changed the title ~~Add dependencies for onnx and onnx-mlir to tt-mlir project linked aga…~~ Add dependencies for onnx-mlir to tt-mlir project linked aga… Aug 2, 2024

uazizTT closed this Aug 2, 2024

uazizTT reopened this Aug 2, 2024

Add support of onnx-mlir project to the existing tt-mlir build. Refac…

3b19a4d

…tor third_party build to accomodate multiple soft dependencies.

uazizTT force-pushed the integrate-onnx-mlir-project-to-tt-mlir branch from c5d652b to 3b19a4d Compare August 2, 2024 13:53

Merge remote-tracking branch 'origin' into integrate-onnx-mlir-projec…

e3b41db

…t-to-tt-mlir

uazizTT force-pushed the integrate-onnx-mlir-project-to-tt-mlir branch from c49499d to d213074 Compare August 3, 2024 00:50

Merge remote-tracking branch 'origin' into integrate-onnx-mlir-projec…

1a321b6

…t-to-tt-mlir

uazizTT force-pushed the integrate-onnx-mlir-project-to-tt-mlir branch from d213074 to 1a321b6 Compare August 3, 2024 03:53

nsmithtt reviewed Aug 3, 2024

View reviewed changes

uazizTT closed this Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dependencies for onnx-mlir to tt-mlir project linked aga… #261

Add dependencies for onnx-mlir to tt-mlir project linked aga… #261

uazizTT commented Jul 31, 2024

nsmithtt Jul 31, 2024

uazizTT Jul 31, 2024

nsmithtt Jul 31, 2024

uazizTT Jul 31, 2024

nsmithtt Jul 31, 2024

uazizTT Jul 31, 2024

nsmithtt left a comment •

edited

Loading

uazizTT commented Jul 31, 2024

nsmithtt Jul 31, 2024

uazizTT Jul 31, 2024

uazizTT Aug 2, 2024

uazizTT left a comment

nsmithtt Jul 31, 2024

nsmithtt Jul 31, 2024

uazizTT Jul 31, 2024

nsmithtt Jul 31, 2024

uazizTT Jul 31, 2024 •

edited

Loading

uazizTT left a comment

uazizTT left a comment

rpavlovicTT commented Aug 1, 2024

gfengTT Aug 2, 2024

nsmithtt Aug 2, 2024

nsmithtt Aug 2, 2024

nsmithtt Aug 2, 2024

uazizTT Aug 2, 2024

nsmithtt Aug 3, 2024

		@@ -1,73 +1,6 @@
		include(ExternalProject)
		option(TTMLIR_ENABLE_ONNX_MLIR "Enable ONNX MLIR support" OFF)

Add dependencies for onnx-mlir to tt-mlir project linked aga… #261

Add dependencies for onnx-mlir to tt-mlir project linked aga… #261

Conversation

uazizTT commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsmithtt left a comment • edited Loading

Choose a reason for hiding this comment

uazizTT commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uazizTT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uazizTT Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

uazizTT left a comment

Choose a reason for hiding this comment

uazizTT left a comment

Choose a reason for hiding this comment

rpavlovicTT commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsmithtt left a comment •

edited

Loading

uazizTT Jul 31, 2024 •

edited

Loading