Skip to content

Conversation

lu-zero
Copy link
Contributor

@lu-zero lu-zero commented Jul 25, 2018

Make the code not scattered between powerpc and powerpc64 and factorize away some redundant target_feature now that the module imports are guarded.

@lu-zero lu-zero requested a review from alexcrichton July 25, 2018 10:05
mod vsx;
#[cfg(target_feature = "vsx")]
Copy link
Contributor

Choose a reason for hiding this comment

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

both vsx and altivec can be detected at run-time, or is this not true for powerpc ?

Still, they can be detected at run-time for powerpc64 right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, the #[target_feature(enable)] are necessary.

If not, we have decided to keep #[target_feature(enable)] attributes on intrinsics even when they are guarded by a static cfg(target_feature).

Changing this should be something that is applied to all backends, not only powerpc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be detected at runtime, but I'm not exactly sure why target_feature should be added to each intrinsic.

Isn't #[target_feature(foo)] a short-hand for #[cfg(target_feature="foo")] ?

Copy link
Contributor

@gnzlbg gnzlbg Jul 25, 2018

Choose a reason for hiding this comment

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

They can be detected at runtime,

So this change breaks that because it forces them to be enabled at compile-time, for the whole binary.

Isn't #[target_feature(foo)] a short-hand for #[cfg(target_feature="foo")] ?

  • #[target_feature(enable = "x")] is a function attribute that tells the compiler to generate code for the function with the feature x enabled.

  • cfg(target_feature = "x") returns true if the feature x has been enabled at compile-time, and otherwise it returns false

So... if cfg(target_feature = "x") is true, then feature x is enabled for the whole binary, so #[target_feature(enable = "x")] is redundant in this case (we still keep it though).

However, if cfg(target_feature = "x") is false, then that just means that the whole binary is being compiled with the feature x disabled. But the CPU that the binary runs on can still support the feature, and you could still do run-time detection and call functions with #[target_feature(enable = "x")] safely on such CPUs.

This changes breaks that because if the binary is not compiled with the features enabled, the intrinsics are just not compiled: #[cfg(...)] does conditional compilation, so it is removing all the code if the condition is false.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The #[cfg(target_feature = "altivec")] comes from 5a20376, I just made the vsx support consistent and I assumed the two attributes are interchangeable and just conditionally compile what they decorate.

Also I assumed that the target_feature would map the same way -maltivec/-mvsx and relative macro symbol do. So it would just signal what the compiler actually supports for the given cpu target.

So, #[target_feature(enable = "foo")] would tell the compiler to use the specific feature notwithstanding what the current target_feature set is? The name is quite misleading if that's the case (and I would just apply it to the whole module anyway)

Copy link
Contributor

@gnzlbg gnzlbg Jul 25, 2018

Choose a reason for hiding this comment

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

The #[cfg(target_feature = "altivec")] comes from 5a20376,

So what that PR does is require altivec to be enabled at compile-time for powerpc builds. Why? I don't really know which problems @alexcrichton run into while doing the update, but an issue should be filled about this and that should be fixed.

Also, this removes the RUSTFLAGS in ci, which now means that altivec is not being tested at all for powerpc.

So, #[target_feature(enable = "foo")] would tell the compiler to use the specific feature notwithstanding what the current target_feature set is?

target_feature(enable) enables a target feature for a function, nothing more, nothing less.

cfg() does conditional compilation, cfg(target_feature = "...") does that based on the target_features enabled for the whole binary in the compiler, e.g., via -C target-feature=+....

The name is quite misleading if that's the case (and I would just apply it to the whole module anyway)

As mentioned, it is a function attribute (so you cannot apply it to a module). It works just like __attribute__ (( __target__ ("x") )) in C.

All of this should be is explained in the top-level docs of this crate (EDIT: https://doc.rust-lang.org/nightly/std/arch/).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if powerpc is still in non-really-building condition, I can keep the additional decorators around assuming they do not work on module-level and continue to implement the other intrinsics.

Even if it is unlikely to happen the VSX instructions can be executed in
32bit mode just as well.
@alexcrichton
Copy link
Member

The current state at least looks good to me, thanks!

@alexcrichton alexcrichton merged commit 3e2efe9 into rust-lang:master Jul 26, 2018
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