Pre-RFC: stabilization of target_feature

[gnzlbg]

I think you’re trying to replicate the exact same features as exist in C when those don’t even work very well.

Unless we write our own compiler backend we are very much restricted with the functionality and semantics that LLVM does supports, which sadly means what clang does support, which again emerged as an effort to be compatible with GCC.

---

@pedrocr I think it would be great if you could re-formulate your proposed solution with the exact semantics on code generation, run-time execution, type-checking, … that you want for each case, since the one that you posted above was a start, but has crystalized more in the discussion. (When you are not clear about the level of detail required, maybe mention to which rust code your abstractions desugar too)

[pedrocr]

@gnzlbg isn’t the block1/block2 case I wrote enough? What else are you looking for or what doubts does that give you?

[gnzlbg]

For the same reason, I don’t think the runtime version of if cfg!(feature) is feasible.

I think this would also depend on whether the generated LLVM IR explicitly use platform specific intrinsics (or even asm! directly), or LLVM generic intrinsics. LLVM can hoist its own intrinsics out of these ifs without problems (although this isn’t probably what the user wanted).

The only fix I can think of is making these functions inline(never), but this is a huge hammer.

[zackw]

For the same reason, I don’t think the runtime version of if cfg!(feature) is feasible.

Oh, why not? In my example it gets converted to function calls and those guarantee the proper boundaries, no?

Yes, if you always outline such blocks to inline(never) functions it should be safe.

I think this would also depend on whether the generated LLVM IR explicitly use platform specific intrinsics (or even asm! directly), or LLVM generic intrinsics. LLVM can hoist its own intrinsics out of these ifs without problems (although this isn’t probably what the user wanted).

All of the above can and will be hoisted, because the compiler is not aware that the feature-requiring instruction has a “dependency” (in the compiler-jargon sense) on the feature check.

The only fix I can think of is making these functions inline(never), but this is a huge hammer.

I’m afraid it is the only hammer in town.

[pedrocr]

Yes, if you always outline such blocks to inline(never) functions it should be safe.

That seems fine to me. To get good speedups just make sure to do the checks sufficiently outside the hotter bits of code to not have a bunch of function call overhead.

[gnzlbg]

@pedrocr you wrote:

But based on the discussion above here’s an integrated proposal:

Compile time detection is done with target_feature like this:

if cfg!(target_feature = “some feature”) { … code with feature … } else { … code without … }

This is clear, if cfg!(target_feature = "some feature") { is a macro that expands at compile-time to either true or false. This enables conditional code generation. Here the question of whether the compiler can hoist target specific instructions out of the if remains open. The compiler shouldn’t do this. Has the same semantics as cfg!(target_feature = "some feature") in the RFC.

Runtime detection is done the same way:

if cfg!(runtime_target_feature = “some feature”) { … code with feature … } else { … code without … }

IIUC this macro expands to some runtime cpuid code, but it doesn’t have any semantics beyond that right? That is, the value of cfg!(target_feature) in both branches is the same. Is this correct? Also, you can call whatever code you like in both branches, nothing is checked at compile-time, at worst you get a segfault. Is this correct? This is basically the same as calling a function from an external crate, like in the cpuid examples of the RFC.

Unconditional feature enablement is also supported but is unsafe. This is your current target_feature and should be a corner case most people will never use.

#[unconditional_target_feature(“some feature”)] fn myfunc() { … code with feature … }

This has the same semantics of #[target_feature] in the RFC.

So IIUC the only change you want is for some sort of run-time detection to be added to the RFC, whether it happens via cfg!(runtime_target_feature = "feature") or std::cpu::has_feature("feature") is just a minor syntactic detail right?

[pedrocr]

IIUC this macro expands to some runtime cpuid code, but it doesn’t have any semantics beyond that right?

It does. In the if branch the feature is enabled for code generation, in the else branch it’s not.

[gnzlbg]

It does. In the if branch the feature is enabled for code generation, in the else branch it’s not.

So what happens when I call a function on the branch where the feature is enabled? Is the feature enabled for that function (e.g. a copy of the function with the feature enabled is generated?). If yes, what does this do when the source code is not available? E.g. on ABI boundaries? Can it somehow select function on #[unconditional_target_feature] ?

If it cannot “see” through functions, and it doesn’t enable cfg!(target_feature), there is no way for the code inside the branches to know and implement feature-dependent behavior. Doesn’t the code look identical in both branches? If yes, how do you avoid this code repetition? If not, what is the difference?

[pedrocr]

So what happens when I call a function on the branch where the feature is enabled?

Nothing, it’s a function call. Unless the feature that you’ve enabled changes something about how to do function calls it will be just the same call.

The code the user writes in both branches is going to be exactly identical or what am I missing?

Sometimes it can be sometimes it will not be. As a programmer you know that the first block has the feature enabled and the second doesn’t and you can take advantage of that. Maybe that’s writing a different algorithm that’s more ammenable to the feature. Maybe that’s using an unsafe intrinsic that you now know will be available…

and it doesn’t enable cfg!(target_feature)

I see what you mean there and maybe enabling that flag will be a good idea. Since you’re in a block that has the feature enabled maybe this makes sense:

if cfg!(runtime_target_feature="avx") {
  if cfg!(target_feature="avx") {
    ... your code here ...
  } else {
    unreachable!();
  }
} else {
  ... some other code here...
}

Now obviously this case makes no sense as you already know the check works but if the target_feature check is somewhere inside a macro that gets expanded out it makes sense that it would evaluate to true when it’s inside the runtime_target_feature true branch.

[gnzlbg]

Maybe that’s writing a different algorithm that’s more ammenable to the feature. Maybe that’s using an unsafe intrinsic that you now know will be available…

How do you abstract this code between run-time and compile-time detection? You want to write this code once, but be able to select the proper branch at either compile-time or run-time.

Now obviously this case makes no sense as you already know the check works but if the target_feature check is somewhere inside a macro that gets expanded out it makes sense that it would evaluate to true when it’s inside the runtime_target_feature true branch.

Can you maybe check the “How do we teach this?” section of the RFC and let me know what you think?

The only difference AFAIK is that instead of using if cfg!(runtime_target_feature it uses if std::cpuid::has_target_feature, but semantically it should be very similar to the point you are coming to.

It’s basically a three layered approach:

• compile-time detection is used to implement the algorithms

• unconditional_target_feature is used to force code generation using particular features

• some run-time detection library is used to dispatch on these unconditional_target_features

And then any kind of sugar that makes this easier can be provided as procedural macros.

[parched]

@gnzlbg, Nice work, just a few comments from me.

Removing features from the default_feature_set using #[target_feature = “-feature_name”] is not allowed.

Does that mean all features that are exposed have to be additive? I.e., for subtractive feature like the arm llvm feature d16 would have to be exposed by rust as +d32 = -d16. (I’d be fine with that renaming). What about mutually exclusive features like +/-thumb-mode on arm? I guess mutually exclusive ones could be their own attributes like #[thumb_mode] and #[arm_mode] like GCC does in this case, however there is already a precedent for mutually exclusive target features on the command line with crt-static. Also if - isn’t allowed then we can remove the + as it’s redundant.

Whether #[target_feature = “feature_name”] requires unsafe fn or not could depend on feature_name.

+1 from me here.

One thing I think this RFC needs to address is how #[target_feature] and cfg(target_feature) should interact. I.e., should cfg(target_feature) know about the extra features if it is used in a #[target_feature] function? IMO, yes. See https://github.com/rust-lang/rust/issues/42515 for some discussion.

[alexcrichton]

Sorry a huge amount of discussion has happened on this thread since I last got a chance to read it over. Is there perhaps a tl;dr; of why the RFC recommends requiring unsafe for #[target_feature] usage? I continue to personally feel that’ll essentially kill SIMD usability in Rust.

[pedrocr]

It’s basically a three layered approach:

• compile-time detection is used to implement the algorithms
• unconditional_target_feature is used to force code generation using particular features
• some run-time detection library is used to dispatch on these unconditional_target_features

And then any kind of sugar that makes this easier can be provided as procedural macros.

These three features are enough to do everything indeed. My only two points are:

1. Having the run-time detection be mandatory for new features that are standardized would be important to not end up with a mix of different levels of compatibility for each feature
2. Having to roll your own feature detection with #[unconditional_target_feature] and the runtime lib instead of having that be the thing that’s standardized sounds a bit like standardizing the low-level pieces and leaving it up to the user to put it together, but maybe that’s fine.

@alexcrichton I think the td;dr; is simply that if you expose in the language something that turns on extra features without runtime checks then at least in some architectures you are allowing programs to be written that will cause memory corruption or other unsafe instances when the feature-enabled code ends up running on a CPU without the feature. I don’t think this is much of an issue though. First that unsafety already exists today on those architectures if you just compile with that feature enabled for the whole program. Second the way the RFC is designed is fairly low level so normal users will end up only interacting with it through crates that abstract away the unsafe with some nicely usable macros.

@parched it also seems to me that having cfg!(target_feature = "avx") == true inside a #[target_feature = "avx"] function makes the most sense.

[gnzlbg]

I continue to personally feel that’ll essentially kill SIMD usability in Rust.

Why do you feel this? To use SIMD in Rust you never need to use target_feature. If you were writing the simd crate, then yes, but if you are just using it, then no. If you are not using the simd crate but rustc intrinsics directly, all of them are unsafe, so I don’t know why you would care either.

---

Having the run-time detection be mandatory for new features that are standardized would be important to not end up with a mix of different levels of compatibility for each feature

I still don’t think this is that critical since we have been able to use all this functionality without standard feature detection just fine. However, if you feel strong about this it is possible to write a small RFC on top of mine motivating it. If enough people feel the same way as you do, we can merge those RFCs and be done with it

[alexcrichton]

Thanks for explaining @pedrocr and @gnzlbg. I remain unconvinced, however. Are there links to concrete examples of where this causes memory unsafety? What platforms does this happen on?

@gnzlbg the decision of safety here is much larger than intrinsics, it’s a definition of SIMD as-a-whole. If we declare avx2 features as “fundamentally unsafe” then the unsafety must propagate outwards to the actual point where you do a runtime dispatch based on available CPU features. We won’t’ be able to hide this under the cover with intrinsics.

[burntsushi]

f you are not using the simd crate but rustc intrinsics directly, all of them are unsafe, so I don’t know why you would care either.

I responded to this before, but maybe you missed it. The vast majority of vendor intrinsics are not unsafe at all.

[gnzlbg]

@burntsushi I see that your stdsimd crate uses the SIMD types and #[target_feature = "..."] to define the intrinsic functions, as opposed to how the simd crate in the nursery does it (which is to call the LLVM intrinsics directly, which is unsafe): https://github.com/rust-lang-nursery/simd/blob/master/src/x86/avx.rs . Since I’ve only used the crate in the nursery, I was talking only about that way of doing things.

1. Are you testing the generated assembly? #[target_feature = "..."] allows the compiler to use those instructions, but the compiler doesn’t need to use them. For example, if you don’t call the intrinsic directly LLVM often doesn’t recognize the code, the intrinsic won’t be emitted, and the wrong instructions are generated (e.g. rbit, but pdep, pext, bzhi, and many other algorithms have this problem). At the same time, even if LLVM emits the correct intrinsic, sometimes it will generate “far from optimal code”, so at least in my bitintr crate I had to start testing the generated assembly and filling LLVM bugs (some of which were answered in IRC by “you need to use the intrinsic directly, LLVM will probably never be able to recognize the algorithms”).

2. To me both ways of implementing a wrapper around compiler intrinsics look functionality-wise identical. Could you comment on the tradeoffs of doing it one way or the other? On a first look is the main differences are that one requires less unsafe than the other, and that because you are relying on LLVM portable vector intrinsics (which is the right thing for SIMD), your code is nicer to read. Anyhow IIRC calling an intrinsic function on a platform that doesn’t support it is undefined behavior per LLVM (some intrinsics are portable though). Since using #[target_feature] allows LLVM to introduce these calls, the old approach was doing the right thing (maybe not in the right way) in assuming that using these intrinsics was unsafe. Note also that just because #[target_feature] could require unsafe fn doesn’t mean that your users will need to write unsafe code, e.g., the nursery/simd crate deals with this by wrapping the unsafe functions in safe ones, so that users of the crate never need to deal with unsafe.

---

@alexcrichton

@gnzlbg the decision of safety here is much larger than intrinsics, it’s a definition of SIMD as-a-whole. If we declare avx2 features as “fundamentally unsafe” then the unsafety must propagate outwards to the actual point where you do a runtime dispatch based on available CPU features. We won’t’ be able to hide this under the cover with intrinsics.

This is only true if your SIMD wrapper exposes this unsafety to the users. The (“old?”) nursery/simd crate uses the unsafe SIMD intrinsics everywhere, but it doesn’t expose this unsafety to the users. This is how the SIMD approach is supposed to look like with this RFC:

// Raw intrinsic function: dispatches to LLVM directly. 
// Undefined behavior to call this
// on an unsupported target
extern "C" { fn raw_intrinsic_function(f64, f64) -> f64; }

// Software emulation of the intrinsic, 
// works on all architectures
fn software_emulation_of_raw_intrinsic_function(f64, f64) -> f64;

// Calling this function is always safe, has zero cost 
// (can be inlined, etc.) see below
fn my_intrinsic(a: f64, b: f64) -> f64 {
  // Note: if the intrinsic is portable just, directly call the intrinsic
  #[cfg!(target_feature = "some_feature")] {
    // if "some_feature" is enabled, it is safe to call the raw intrinsic function
    unsafe { raw_intrinsic_function(a, b) }
  }
  #[not(cfg!(target_feature = "some_feature"))] {
     // if "some_feature" is disabled calling 
     // the raw intrinsic function is
     // undefined behavior (per LLVM), we call the software emulation
     software_emulation_of_raw_intrinsic_function(a, b)
  }
}

// Provide run-time dispatch to the best implementation:
// ifunc! is a procedural macro that generates copies 
/// of `my_intrinsic` with different target features,
// does run-time feature detection on binary initialization, 
// and sets my_intrinsic_rt 
// to dispatch to the appropriate implementation.
static my_intrinsic_rt = ifunc!(my_intrinsic, ["default_target", "some_feature"]);
// Note that because `#[target_feature = "some_feature"] fn foo; sets 
// cfg!(target_feature = "some_feature") to true, the right 
// branch within my intrinsic will be taken. 

// This procedural macro expands to the following:

fn initialize_my_intrinsic_fn_ptr() {
  if std::cpuid::has_feature("some_feature") -> typeof(my_intrinsic) {
    // using my_intrinsic_some_feature_wrapper is unsafe in general, 
    // but because we have made the run-time feature test we have made it safe:
    unsafe { my_intrinsic_some_feature_wrapper /* do we need a cast here?: as typeof(my_intrinsic) */ }
  } else {
    my_intrinsic
  }
}

// The macro copies the tokens of `my_intrinsic` 
// and annotates the function with #[target_feature]
#[target_feature = "some_feature"]
unsafe fn my_intrinsic_some_feature_wrapper(a: f64, b: f64) 
{
  #[cfg!(target_feature = "some_feature")] {
    // this branch will always be taken because 
    // `#[target_feautre = "..."]` defines `cfg!(target_feature = "...") == true`
    unsafe { raw_intrinsic_function(a, b) }
  }
  #[not(cfg!(target_feature = "some_feature"))] {
     // dead code
     software_emulation_of_raw_intrinsic_function(a, b)
  }
}

Maybe it would make sense to compare this with the approach that @burntsushi is proposing, and weight pros/cons. But I really still don’t understand how #[target_feature = "..."] unsafe fn kills the SIMD story given that we already have a SIMD crate (nursery/simd) for which this is not an issue at all.

---

EDIT: an updated version of this example can be found on this comment of the RFC PR.

[burntsushi]

I see that your stdsimd crate uses the SIMD types and #[target_feature = “…”] to define the intrinsic functions, as opposed to how the simd crate in the nursery does it (which is to call the LLVM intrinsics directly, which is unsafe)

They aren’t as different as you might imagine. Look at _mm_adds_epi16, for example. You’ll notice that it is calling the LLVM intrinsic. The simd crate is effectively doing the same thing, but through the platform-intrinsics machinery in rustc. stdsimd bypasses platform-intrinsics completely. platform-intrinsics are insufficient for exposing vendor intrinsics.

Are you testing the generated assembly? #[target_feature = “…”] allows the compiler to use those instructions, but the compiler doesn’t need to use them.

The plan is to add these tests later because rustc already has some infrastructure for doing this, and I don’t understand it. Thus far, I’ve been ad hoc looking at the generated Assembly.

Note that this is how Clang exposes vendor intrinsics. I am not taking a novel path here. This is well trodden ground.

To me both ways of implementing a wrapper around compiler intrinsics look functionality-wise identical.

They aren’t? The simd crate is exposing a high level interface to SIMD functionality. stdsimd, on the other hand, is explicitly providing an implementation of the vendor intrinsic interfaces defined by CPU vendors. stdsimd is modeled on how Clang provides the same interface. I’d encourage you to compare and contrast: https://github.com/llvm-mirror/clang/blob/master/lib/Headers/emmintrin.h

e.g., the nursery/simd crate deals with this by wrapping the unsafe functions in safe ones, so that users of the crate never need to deal with unsafe.

This really isn’t a question of whether or not a high level SIMD library should be unsafe or not, and I’m really confused as to why you’re focusing on that. We are specifically talking about explicit use of #[target_feature], which I imagine will be commonly used in conjunction with specific vendor intrinsics. Vendor intrinsics must be defined with #[target_feature] in order for anything to work at all.

[gnzlbg]

platform-intrinsics are insufficient for exposing vendor intrinsics.

I see, thanks for chiming in.

The plan is to add these tests later because rustc already has some infrastructure for doing this, and I don’t understand it. Thus far, I’ve been ad hoc looking at the generated Assembly.

If you ever get to it let me know. I did not understand those either and quickly hacked my own tiny system for it that just does a 1:1 comparison of the generated assembly. If you ever use this for the stdsimd crate let me know, I’ll be glad to dump mine and use what rustc uses.

Note that this is how Clang exposes vendor intrinsics. I am not taking a novel path here. This is well trodden ground.

Note that calling a function with __target__ in a CPU that doesn’t support its feature set seems to be undefined behavior (in LLVM, the assembler, and probably the CPU as well). See this comment on the RFC PR.

Basically, not even x86 guarantees the illegal instruction will be reached at all, nor that the decoder will detect that it is an illegal instruction, nor that the assembler didn’t insert other target specific code before the instruction that in a different CPU might lead to memory unsafety.

This really isn’t a question of whether or not a high level SIMD library should be unsafe or not, and I’m really confused as to why you’re focusing on that.

I thought that stdsimd was a safe wrapper for SIMD intrinsics but IIUC it is only a proposal for the low-level intrinsics that std should expose, such that high-level wrappers can be built.

Vendor intrinsics must be defined with #[target_feature] in order for anything to work at all.

Not all of them. Maybe this is common for SIMD, but for the bit manipulation instruction sets (TBM, ABM, BMI1, and BMI2) this is not required. Calling the llvm intrinsic directly, like here for rbit or here for pdep, is enough.

---

@alexcrichton I think after this explanation from @burntsushi I finally understand what you mean with making #[target_feature] unsafe would significantly impact our SIMD story. I think this is an issue, but I am still not convinced that this is a big / critical one. Here is why:

The purpose of putting the lowest-level-possible SIMD intrinsics in std:: is to allow SIMD libraries to be written in stable rust out of tree. If the std:: intrinsics for SIMD are unsafe, those writing a wrapper around them will need to deal with this unsafety. These intrinsics can be wrapped in “low-lever” libraries out of std:: that provide a safe API without introducing an extra cost.

So unless one is writing a “lowest-level-safe SIMD wrapper” one shouldn’t need to deal with this. Even those writing slightly higher-level should build on top of safe-low-level wrappers.

Also, we can always make these intrinsics safe in a backwards compatible way.

What we cannot do is make these intrinsics unsafe after we made them safe in a backwards compatible way.

I really hope that I am missing something, and that @alexcrichton or @burntsushi can convince me that either the impact to the ecosystem will be significant or that it is 100% safe to call these intrinsics on targets that do not support them. After talking with some LLVM-devs about how decoders on Intel CPUs cannot be relied at all to raise SIGILL exceptions and how the assembler might insert filling code before that happens anyways that might modify memory I still lean on the safest solution which is to make #[target_feature] unsafe.

[burntsushi]

Note that calling a function with target in a CPU that doesn’t support its feature set seems to be undefined behavior (in LLVM, the assembler, and probably the CPU as well). See this comment on the RFC PR.

Yeah, I saw that. That may make this quibbling moot. :-/

I thought that stdsimd was a safe wrapper for SIMD intrinsics but IIUC it is only a proposal for the low-level intrinsics that std should expose, such that high-level wrappers can be built.

It’s two things I guess. The goal is to expose a small platform independent API consisting of SIMD vector types, while also providing a means to expose platform dependent vendor APIs that interoperate with aforementioned SIMD vector types. Both things are, IMO, required for a SIMD ecosystem to grow. Safety for the platform dependent APIs isn’t the primary objective. Making the intrinsics available at all is the primary objective. If they have to be unsafe, then so be it, but up until your recent post on the RFC, my understanding was that most of the vendor intrinsics would be entirely safe to call (assuming we straighten out the ABI issues).

Not all of them. Maybe this is common for SIMD, but for the bit manipulation instruction sets (TBM, ABM, BMI1, and BMI2) this is not required. Calling the llvm intrinsic directly, like here for rbit or here for pdep, is enough.

That is interesting. I guess that only works if the LLVM intrinsic doesn’t require any SIMD vector types? Otherwise I think LLVM will complain. It also might be something will stop working over time if LLVM ever removes the explicit intrinsic. In particular, LLVM does not have intrinsics for every vendor intrinsic and instead relies on code generation. In that case, I think you will need #[target_feature] even if you aren’t using any SIMD vectors.