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 macos-14 (arm64) CI (and made it pass finally) #278

Closed
wants to merge 2 commits into from

Conversation

cielavenir
Copy link
Contributor

@cielavenir cielavenir commented Mar 1, 2024

CI passed according to https://github.com/cielavenir/isa-l/actions/runs/8114714856

@cielavenir
Copy link
Contributor Author

Extended tests passed as well https://github.com/cielavenir/isa-l/actions/runs/8130749368/job/22219331109

@cielavenir cielavenir changed the title Add macos-14 CI (and made it pass finally) Add macos-14 (arm64) CI (and made it pass finally) Mar 3, 2024
@pablodelara
Copy link
Contributor

HI @cielavenir. Thanks for your PR! Can you clean it up a little bit? There are a bunch of "merge" and "revert" patches that are polluting the PR. Also, since you have integrated the fix in PR #226, can you close that PR?

@cielavenir cielavenir force-pushed the macosCI branch 2 times, most recently from 1616793 to bd7fe6c Compare March 4, 2024 13:43
@cielavenir
Copy link
Contributor Author

@pablodelara Squashed

@cielavenir
Copy link
Contributor Author

by the way, not sure I'm noob at github, but how can I run CI without patching on: field? I'm asking because this caused pushing back and forth.

@pablodelara
Copy link
Contributor

Thanks @cielavenir! What I meant is to keep only the commits that are adding value overall, and remove the "merge" commits. Looking at the PR, you could have 4-5 commits, each one adding/fixing some functionality. E.g.:

  • Add macos14
  • Fix mach compilation on MacOS
  • Avoid x18 (register?)
  • Fixed isal_deflate_icf_finish_lvl1 dispatcher

@cielavenir
Copy link
Contributor Author

Actually is it possible to merge #226 and #280 then rebase this pull request again?

@pablodelara
Copy link
Contributor

@cielavenir Well, those PR's are closed. We could reopen them, but still this PR should be split into several commits.

@cielavenir
Copy link
Contributor Author

right please reopen and merge them

@cielavenir
Copy link
Contributor Author

@pablodelara @liuqinfei Now rebased again. The thing is, #226 and #280 are obvious but avoiding x18 register should be reviewed carefully as I rely only on CI result.

@pablodelara
Copy link
Contributor

@liuqinfei can you review this PR?

@liuqinfei
Copy link
Contributor

liuqinfei commented Mar 14, 2024 via email

@liuqinfei
Copy link
Contributor

I think the change is ok for Aarch64 platform. And, i verified this PR on kunpeng 920 (Aarch64), all the test cases pass as following.

Snipaste_2024-03-14_23-35-15

Signed-off-by: Taiju Yamada <tyamada@bi.a.u-tokyo.ac.jp>
Signed-off-by: Taiju Yamada <tyamada@bi.a.u-tokyo.ac.jp>
@cielavenir
Copy link
Contributor Author

rebased again.

now let's see if macOS M1 extended tests pass.

@pablodelara
Copy link
Contributor

This is merged now, thanks!

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