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

Implement all MSA Intrinsics #709

Merged
merged 3 commits into from
Apr 9, 2019
Merged

Implement all MSA Intrinsics #709

merged 3 commits into from
Apr 9, 2019

Conversation

rbirdic
Copy link
Contributor

@rbirdic rbirdic commented Apr 4, 2019

Fixes #170

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

This needs a thorough review, but two things stand out:

  • the msa module should use newtype vector types instead of re-exporting the ones from the simd module
  • this is not built in CI AFAICT, so this will need at least CI that builds with msa enabled (a qemu build job that runs the tests on CI can be added in a subsequent PR though)

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 5, 2019

This is looking really good, thank you for working on this.

Do you think you would be able to add a couple of buildjobs that build the library with -C target-feature=+msa enabled ? That way we can at least ensure that this compiles and that we don't break it. Ideally, we would be able to run the tests under qemu as well, but that can be done in a follow up PR.

@rbirdic
Copy link
Contributor Author

rbirdic commented Apr 5, 2019

@gnzlbg Can I repurpose existing MIPS buildjobs for that or do I need to add new ones?

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 5, 2019

You can repurpose them, by adding a case for mips here (https://github.com/rust-lang-nursery/stdsimd/blob/master/ci/run.sh#L64) and then enabling MSA.

I think you might want to add new build jobs for the new r6 targets though.

@rbirdic
Copy link
Contributor Author

rbirdic commented Apr 8, 2019

I think you might want to add new build jobs for the new r6 targets though.

Rustc changes which provide mips r6 support have been recently added (1.35-nightly). I don't think there is rustup target for mipsisa64r6*-unknown-linux-gnuabi64 available.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 8, 2019

@rbirdic rustup is not required, you can use xargo to recompile core for those targets, the thumb..., ptx, and gba build jobs do just that.

@wzssyqa
Copy link

wzssyqa commented Apr 9, 2019

@rbirdic in ci/run.sh, you seem not taking care about r6:
mipsisa{32,64}r6{,el}

@wzssyqa wzssyqa mentioned this pull request Apr 9, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 9, 2019

@wzssyqa a *mips*) case should be enough for that

@gnzlbg gnzlbg merged commit 5986587 into rust-lang:master Apr 9, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 9, 2019

So this looks good enough to merge for me. Build jobs for the r6 targets can be added later.

@rbirdic
Copy link
Contributor Author

rbirdic commented Apr 9, 2019

@wzssyqa

@rbirdic in ci/run.sh, you seem not taking care about r6:
mipsisa{32,64}r6{,el}

You're right, I somehow missed it.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 9, 2019

How soon do you need these intrinsics on nightly Rust?

I would be more comfortable if we were to set up running the run-time tests on CI before landing these upstream.

@rbirdic
Copy link
Contributor Author

rbirdic commented Apr 10, 2019

I am ok with adding test first.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 10, 2019

@rbirdic I've opened a PR with some fixes #711 , we missed that all APIs here are private, and are not exposed by the library :D

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