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

mac m1 cross compile #1204

Merged
merged 9 commits into from
Aug 10, 2022
Merged

mac m1 cross compile #1204

merged 9 commits into from
Aug 10, 2022

Conversation

powderluv
Copy link
Collaborator

@powderluv powderluv commented Aug 9, 2022

Add support for M1 cross compile. Also compile and test macos x86

Copy link
Collaborator

@makslevental makslevental left a comment

Choose a reason for hiding this comment

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

CONGRATS

Copy link
Member

@sjain-stanford sjain-stanford left a comment

Choose a reason for hiding this comment

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

I think Run torch-mlir unit tests step is still using x86_64 build for running tests:

cmake --build build --target check-torch-mlir-all

Is this intentional? Or do we want to configure this with _${{ matrix.targetarch }} as well?

image

Copy link
Collaborator Author

@powderluv powderluv left a comment

Choose a reason for hiding this comment

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

Yeah we are building both architectures and testing only x86

@powderluv powderluv merged commit 2342456 into main Aug 10, 2022
@powderluv powderluv deleted the powderluv-m1-cross-compile branch August 10, 2022 15:48
Copy link
Member

@sjain-stanford sjain-stanford left a comment

Choose a reason for hiding this comment

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

Yeah we are building both architectures and testing only x86

This makes sense. The only consideration is the 2G ccache limit. As we're building twice in the same job / runner, we may always see low cache hit rate? Is it better to set the cross compile build as a separate github workflow? This will also maximize parallelization (use a separate runner of which there are no restrictions I presume)? I'm hesitant in increasing the ccache limit of 2G more due to the reasons we discussed on discord.

@powderluv
Copy link
Collaborator Author

I think we can drop the macOS x86 build all together. I think we can now reduce the matrix considerably.

@sjain-stanford
Copy link
Member

I think we can drop the macOS x86 build all together. I think we can now reduce the matrix considerably.

You mean remove x86 build + tests for macos and only use arm64 build + tests? That would be great indeed :)

sjain-stanford added a commit that referenced this pull request Aug 11, 2022
Addresses #1207. 

#### Provisioned jobs:
```
# ubuntu - x86_64 - llvm in-tree     - pytorch binary - build+test    # most used dev flow and fastest signal
# ubuntu - x86_64 - llvm out-of-tree - pytorch source - build+test    # most elaborate build
# macos  - arm64  - llvm in-tree     - pytorch source - build only    # cross compile, can't test arm64
```

#### Main changes
- [x] Spawn macos builds from a separate matrix (in the same workflow). It made sense to do this as they are fairly different from ubuntu (cross compile, use a different cmake configuration). This simplifies the matrix configuration and exclusions quite a bit, and makes the workflow a bit more tractable and maintenance friendly.
- [x] Remove the submodule md5sum step for ccache config. This was [broken](https://github.com/llvm/torch-mlir/runs/7779288734?check_suite_focus=true#step:3:145) for a while now.
- [x] Removes unused matrix options - `os`, `targetarch`, `python-version`, `llvmtype`.
- [x] Address ZSTD [comment](#1204 (comment)) on @powderluv's cross compile [PR](#1204). 

#### Further improvements (to be addressed in follow-on):
* ubuntu-x86_64 out-of-tree integration tests fail ([error](https://github.com/sjain-stanford/torch-mlir/runs/7781264029?check_suite_focus=true)); only run unit tests for now (tests are excluded in current CI too)

#### Passing workflow:
https://github.com/sjain-stanford/torch-mlir/actions/runs/2840676309
![image](https://user-images.githubusercontent.com/19234106/184194535-f3807991-401a-4cb9-b030-0ee8c334eba3.png)
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
Signed-off-by: Ettore Tiotto <etiotto@ca.ibm.com>
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