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

[mips/mips64: msa] add add_a_b intrinsic #365

Merged
merged 27 commits into from
Mar 10, 2018

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Mar 9, 2018

This PR adds a single mips::msa intrinsic (add_a_b: vector add absolute values).

It adds all the scaffolding for adding more mips/mips64 intrinsics "easily" in the future.

  • adds an arch::mips64 module
  • generates documentation for mips/mips64
  • adds run-time feature detection for mips/mips64 (auxiliary vectors only)
  • adds simd_test support for mips/mips64 (requires parsing instructions as expressions instead of identifiers because add_a.b is a valid MIPS64 instruction name)

The following issues are left for future PRs:


@alexcrichton I cleaned up the run-time feature detection on linux a bit because it was getting out-of-hand. I have no idea whether this might break rustc builds or not. Is there a way for me to test this? If these changes break some build bots there, we should add these build bots to our CI as well.

cc @jcowgill

@gnzlbg gnzlbg changed the title [mips64/msa] add add_a_b intrinsic [mips/mips64: msa] add add_a_b intrinsic Mar 9, 2018
@gnzlbg gnzlbg requested a review from alexcrichton March 9, 2018 15:20
@gnzlbg gnzlbg mentioned this pull request Mar 9, 2018
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good! My main comment is to try to reduce the amount of #[cfg] as it looks like it'd be pretty difficult to otherwise maintain over time

.travis.yml Outdated
- ci/run-docker.sh $TARGET $FEATURES
- |
if [ "$NORUN" == "1" ]; then
cd crates/stdsimd
Copy link
Member

Choose a reason for hiding this comment

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

Could this use the -p argument to avoid changing the cwd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

use core::mem;
use _std::prelude::v1::*;
use _std::fs::File;
use _std::io::Read;
Copy link
Member

Choose a reason for hiding this comment

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

This unfortunately will cause some problems when building in libstd itself as this doens't exist, but could this continue to use the same strategy as before?

}

#[cfg(not(target_os = "linux"))]
Copy link
Member

Choose a reason for hiding this comment

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

I'm personally not a huge fan of #[cfg] as it can make already complicated code even more complicated, so could this continue to compile on all platforms and just statically return an error on the "wrong" platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC the problem here was that the auxvec/cpuinfo modules are only available on some oses (currently only linux), but these functions should be available on all OSes and just return nothing on linux.

So IMO the entanglement here is that we have target_os modules (e.g. linux) and arch modules (e.g. aarch64) and a mixture of archs and target_oses on both. I'll try to think of something better here.

use _std::fs::File;
use _std::io::Read;

#[cfg(not(all(target_os = "linux",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps cfg_if! could be used to simplify this? That could be added as a dependency and it's available in libstd as well

target_arch = "mips64"))]
pub hwcap: usize,
#[cfg(any(target_arch = "arm", target_arch = "powerpc64"))]
pub hwcap2: usize,
Copy link
Member

Choose a reason for hiding this comment

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

Could both fields be perhaps unconditionally included to avoid the extra #[cfg] traffic?


#[cfg(any(target_arch = "aarch64", target_arch = "arm",
target_arch = "powerpc64", target_arch = "mips",
target_arch = "mips64"))]
Copy link
Member

Choose a reason for hiding this comment

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

Ideally I'd prefer to avoid the #[cfg] here, it becomes a pretty big burden to maintain over time :(

.travis.yml Outdated
- ci/run-docker.sh $TARGET $FEATURES
- |
if [ "$NORUN" == "1" ]; then
cd -p crates/stdsimd
Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry I meant -p arguments to the cargo commands :)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 9, 2018

@alexcrichton So I think I managed to clean up cfg-hell at least a bit. The gist in a nutshell is:

stdsimd/detect/ now has two sub-directories:

  • stdsimd/detect/arch/{target_arch}.rs: provides the Feature enum and the is_{target_arch}_feature_detected! macros - these are arch-dependent, but os independent.

  • stdsimd/detect/os/{target_os}.rs: defines the platform-dependent check_for function, that is, how features are actually detected on the different platforms (os + arch).

I don't think this is great, but I think it is better than what we had. Now, cpuinfo, auxvec, and the cache are only defined on the platforms that actually need them. For example, on non-x86 non-linux platforms we don't need the cache because we can't detect any features anyways.

@alexcrichton alexcrichton merged commit 5f74dbd into rust-lang:master Mar 10, 2018
@alexcrichton
Copy link
Member

Alright looks good to me, thanks! We can always of course continue to tweak this over time

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 10, 2018 via email

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.

2 participants