Pre-RFC: Consider statically-enabled target-features when deciding if calling #[target_feature(enable = "foo")] requires unsafe

[hsivonen]

Summary

Currently, calling a function annotated with #[target_feature(enable = "foo")] requires the function making the call also to be annotated with #[target_feature(enable = "foo")] in order for the call not to require unsafe even if "foo" is enabled for the whole compilation. This RFC removes the requirement for unsafe to call a function annotated with #[target_feature(enable = "foo")] when "foo" is enabled for the whole compilation (by any mechanism: --target, -C target-cpu, -C target-feature).

Motivation

Consider the following code that uses u16x8 and u8x16 from core::simd:

cfg_if! {
    if #[cfg(all(target_feature = "sse2", target_arch = "x86_64"))] {
        use core::arch::x86_64::_mm_packus_epi16;
    } else if #[cfg(all(target_feature = "sse2", target_arch = "x86"))] {
        use core::arch::x86::_mm_packus_epi16;
    } else {

    }
}

// We don't care what the output is for lanes whose value is above
// `0xFFu16` as long as it's not UB and doesn't cross-contaminate
// lanes whose value is between `0u16` and `0uFFu16` (inclusive).
//
// Direct use of `_mm_packus_epi16` avoids the cost of zeroing the
// high halves.
cfg_if! {
    if #[cfg(target_feature = "sse2")] {

        #[inline(always)]
        pub fn simd_pack(a: u16x8, b: u16x8) -> u8x16 {
            unsafe {
                // Safety: We have cfg()d the correct platform
                _mm_packus_epi16(a.into(), b.into()).into()
            }
        }

    } else {

        #[inline(always)]
        pub fn simd_pack(a: u16x8, b: u16x8) -> u8x16 {
            let first: u8x16 = a.to_le_bytes();
            let second: u8x16 = b.to_le_bytes();
            let (ret, _) = first.deinterleave(second);
            ret
        }
    }
}

The code requires unsafe even though the compiler could logically be made to figure out on its own that the call to _mm_packus_epi16 is safe when sse2 is enabled for the whole compilation.

The point of unsafe is to let the programmer assert to the compiler that something that the compiler can't itself prove to be safe is indeed safe.

Requiring unsafe when the compiler could be made to figure out on its own that the code is safe is bad, because it dilutes the review value of unsafe.

Most trivially, this makes crates look bad when quickly assessed simply by counting the instances of unsafe. Less trivially, logically unnecessary unsafe pollutes unsafe reviews. In the example above, it looks like it's no big deal. However, things add up: Prior to RFC 2396, pretty much everything using core::arch was unsafe to the point of approaching the C and C++ situation where actually-safe and actually-unsafe were not usefully distinguished as spots to review. RFC 2396 greatly improved the situation; this is the logical next step.

Note that simd_pack itself is deliberately not and should not be annotated with #[target_feature(enable = "foo")]: When programming with core::simd, either no function is annotated with #[target_feature(enable = "foo")] when relying on relevant features being enabled for the whole compilation or only a function at multiversioning point is annotated with #[target_feature(enable = "foo")] and the rest are inlined into it with #[inline(always)], so the relevant set of target_features in effect for the #[inline(always)] takes effect after inlining and the given #[inline(always)] function can get compiled with differet target_features within the same compilation.

Guide-level explanation

In addition for a safe function annotated with #[target_feature(enable = "foo")] not requiring unsafe to call when the caller function also has #[target_feature(enable = "foo")], unsafe is not required when calling a safe function annotated with #[target_feature(enable = "foo")] when "foo" is enabled for the whole compilation.

Reference-level explanation

See the previous section.

Drawbacks

Whether a function call requires unsafe now depends on the target configuration (--target, -C target-cpu, -C target-feature), so a crate tested with one target configuration may fail to compile with an error complaining about missing unsafe when compiled with another target configuration. On the flip side, compiling with warnings as errors may cause a crate not to compile when compiling with a configuration that enables more target_features compilation-wide than the configuration the crate has been tested with.

Rationale and alternatives

The above drawback is in practice much less of a drawback than it theoretically appears.

It's been argued that in the status quo, unsafe is a signal about a portability hazard. In general, that's not what unsafe means: unsafe means that the programmer asserts to the compiler that something that the compiler can't prove to be safe is actually safe. It's not generally a portability concern.

Ever since 1.0 Rust hasn't required unsafe to signal portability hazards when using std::os: the portability hazard signal is the module path (std::os), not unsafe. In this case, the signal of portability hazard is core::arch (or cfg). (The core::arch signal might not be visible for functions defined by crates rather than defined by the standard library, but neither is std::os.)

In practice, code that reaches for core::arch is likely to to use conditional compilation with cfg in one way or another. It is much more likely that a crate developer makes a mistake such that the different conditional pieces don't fit together with all possible target configurations and compilation fails for that reason than for compilation to fail because the crate developer only tested a scenario where unsafe isn't required due to the target configuration and then someone else compiles a crate with a target configuration that requires unsafe. For a user of a crate, the significant thing is the failure the compile the crate, not the specific reason.

Notably, this concern is moot for the code pattern seen as the motivating example. Furthermore, the most mainstream targets where varying target configurations are relevant are variations of x86 and x86_64, and, as also, seen in the example, due to the way core::arch is organized for these architectures, they already pose the portability hazard that it's easy to cause a crate not to compile on x86 due to forgetting to make the import of every function under core::arch conditional to import it either from core::arch::x86_64 or core::arch::x86. Thus, there is already a more likely hazard resulting in failure to compile a crate where not tested compared to the compilability hazard introduced here.

There's an argument to be made that this proposal does not fully remove unsafe, because unsafe is still needed at the run-time dispatch point. unsafe indeed belongs there, but that point does not need to be present in code that uses core::arch. Code that relies statically on cfg, like the motivating example, does not have a run-time dispatch point at all. This is particularly relevant to sse2 on x86_64 and i686 targets and neon on aarch64 targets. Even in code that does use run-time dispatch can delegate the dispatch point to another crate, such as multiversion.

Unresolved questions

Unsolved if the case where unsafe is used but is unnecessary due to (and only due to) the target configuration should be special-cased in the compiler not to emit a warning.

Future possibilities

In the future, add a higher-level way than mere constant propagation to attach target_feature_available_at_call_site to a block and remove the requirement for unsafe to call functions with the checked target_feature from within such a block.

[RalfJung]

Thanks for writing this up! That example is quite illuminating. I think an alternative way to phrase your proposal is that you are basically asking for flow-sensitive target feature checking wrt cfg -- if we are inside a block that's gated on #[cfg(target_feature = "sse2")], we shouldn't need unsafe to call "sse2" functions. The ideal implementation of this would actually realize that we are inside such a block. That would avoid the portability issues. However, it's quite unclear how one would actually do that, so as an approximation of the desired behavior, you are suggesting that we allow "sse2" functions to be called safely whenever target_feature = "sse2" holds, even if we didn't actually check that.

Is this a fair summary? Or do you think that it is actually desirable for "sse2" calls to be safe even if we did not check cfg(target_feature = "sse2"), just because we happen to be in a compilation unit where target_feature = "sse2" holds?

@hsivonen

Note that simd_pack itself is deliberately not and should not be annotated with #[target_feature(enable = "foo")]

Doesn't this mean that _mm_packus_epi16 can't be inlined into simd_pack, since a callee with more target features cannot be inlined into a caller with fewer target features?

(In this specific case a sufficiently smart compiler could do the inlining but LLVM isn't smart enough.)

@hsivonen

On the flip side, compiling with warnings as errors may cause a crate not to compile when compiling with a configuration that enables more target_features compilation-wide than the configuration the crate has been tested with.

Does this refer to the "unnecessary unsafe" warning? It'd be good to make that explicit as it is not obvious.

@hsivonen

The above drawback is in practice much less of a drawback than it theoretically appears.

The discussion that follows is confusing. It seems to argue against the other arguments you were given on Lobsters, not the drawback listed here. Only a few paragraphs down does the drawback listed here become the subject of this discussion.

I agree that unsafe is not used to mark portability hazards. I don't know anyone in the Rust project who ever suggested otherwise, at least recently. However, the drawback of your proposal is that it introduces itself a new portability hazard, and that is a significant downside IMO.

@hsivonen

In practice, code that reaches for core::arch is likely to to use conditional compilation with cfg in one way or another. It is much more likely that a crate developer makes a mistake such that the different conditional pieces don't fit together with all possible target configurations and compilation fails for that reason than for compilation to fail because the crate developer only tested a scenario where unsafe isn't required due to the target configuration and then someone else compiles a crate with a target configuration that requires unsafe. For a user of a crate, the significant thing is the failure the compile the crate, not the specific reason.

That is, I think, the key claim that in my eyes you are making on pure faith -- that portability issues due to incorrect cfg are much more common than the portability issues your proposal introduces.

[newpavlov]

@RalfJung

The ideal implementation of this would actually realize that we are inside such a block. That would avoid the portability issues. However, it's quite unclear how one would actually do that

I think my old proposal is relevant here:

[Pre-Pre-RFC] Target restriction contexts

The following text is braindump-like description of how I would like to see Rust work with various target options. (target_arch, target_feature, target_os and others) This approach aims to provide stronger compile time guarantees compared to current cfg setups, remove most of the unsafe in the stdsimd crate and integrate target parameters closer with Cargo.toml. Also it could be useful for improving Rust portability story. While RFC 2045 is an important step towards stabilization of SIMD, I don…

[hsivonen]

@RalfJung

I think an alternative way to phrase your proposal is that you are basically asking for flow-sensitive target feature checking wrt cfg -- if we are inside a block that's gated on #[cfg(target_feature = "sse2")], we shouldn't need unsafe to call "sse2" functions. The ideal implementation of this would actually realize that we are inside such a block. That would avoid the portability issues. However, it's quite unclear how one would actually do that, so as an approximation of the desired behavior, you are suggesting that we allow "sse2" functions to be called safely whenever target_feature = "sse2" holds, even if we didn't actually check that.

Is this a fair summary?

That would work for me, yes.

@RalfJung

Or do you think that it is actually desirable for "sse2" calls to be safe even if we did not check cfg(target_feature = "sse2"), just because we happen to be in a compilation unit where target_feature = "sse2" holds?

Primarily, I'm expecting that it's easier to implement a check "what target-features are enabled for this compilation?" than to implement a check "is this block gated on #[cfg(target_feature = "sse2")]?", so I expect the former approach to be a more modest request that could result in an implementation shipping sooner.

Secondarily, it seems that the answer to "Could the compiler prove that this is safe?" is clearly "Yes" when looking at what's enabled for the whole compilation unit, so in that sense, it seems that lifting the unsafe requirement only behind a cfg gate would still over-require unsafe relative to what the compiler could easily be taught to prove on its own.

Thirdly, I think writing portable code shouldn't be a requirement, although it's useful to make it so easy to write portable code that people do. (Like the Rust file system API is so good that folks working on Unix-like systems end up writing code that's "for free" closer to working on Windows compared to what they'd end up with if they used C.)

@RalfJung

Doesn't this mean that _mm_packus_epi16 can't be inlined into simd_pack, since a callee with more target features cannot be inlined into a caller with fewer target features?

(In this specific case a sufficiently smart compiler could do the inlining but LLVM isn't smart enough.)

Inlining does happen. However, just now that I prepared a Godbolt link to paste here to show it, I learned that my SSE2-specific version now generates worse code than the portable version. When I originally wrote it, it generated better code. I filed an issue and I find it troubling that the testing situation is such that this kind of regression can occur.

@RalfJung

Does this refer to the "unnecessary unsafe" warning?

Yes.

@RalfJung

That is, I think, the key claim that in my eyes you are making on pure faith -- that portability issues due to incorrect cfg are much more common than the portability issues your proposal introduces.

Not pure faith, but personal anecdata, which is a bit better, though, admittedly, not strong research.

When I've used core::arch, I've done so immediately inside a cfg_if! block, so I haven't messed that part up. However, I have many times messed up how my various functions that have portable signatures and inside contain these core::arch adaptations and are declared behind multiple cfg_if! with different criteria, since different operations need specialization on a different targets, fit together.

[RalfJung]

@hsivonen

That would work for me, yes.

Thanks, that is helpful. Personally I find this a better way to motivate this proposal.

@hsivonen

Secondarily, it seems that the answer to "Could the compiler prove that this is safe?" is clearly "Yes" when looking at what's enabled for the whole compilation unit, so in that sense, it seems that lifting the unsafe requirement only behind a cfg gate would still over-require unsafe relative to what the compiler could easily be taught to prove on its own.

We could do many ad-hoc changes to rustc to recognize some more code as safe, so I don't find this argument very convincing. I find your example convincing, but mostly because it would be accepted by a flow-sensitive analysis, not just because it is sound and we could teach rustc to accept it.

@hsivonen

Thirdly, I think writing portable code shouldn't be a requirement,

It's not about a requirement to make things portable, it's just about nudging people in the direction of portable code. There's a cost to introducing more sources of non-portability and a benefit to accepting more code as safe. I find it non-obvious to weigh the two against each other, so if we can get 80% of the benefit with 0% of the cost that seems like a good deal to me.

But I also can't imagine an easy way to actually implement this flow-sensitive check, so I find it mostly useful as a motivation for what we are approximating, not as the actual proposal.

@hsivonen

Inlining does happen.

Ah right, simd_pack actually does have the target feature, just ambiently via the target or -C flags. (Things will get messy if you mix different -C flags for different parts of the compilation so I will assume that you don't do that.)

I find it troubling that the testing situation is such that this kind of regression can occur.

stdarch is extensively tested, but it's obviously impossible to test every possible way of invoking every possible intrinsic. If you have constructive suggestions for how to catch more such issues, you are welcome to propose them.