Portability checking and `target_feature`

[Ericson2314]

The portability lint as planned just lints the code that did not get cfg'd away. But an extension (raised many time including by me in https://github.com/rust-lang-nursery/portability-wg/issues/8#issuecomment-371246725) would have it be done on everything. This entails delaying the pruning of cfg-dead code until after name resolution, or even type checking.

I always assumed this would be a long ways off, but I just realized something vary similar has been proposed with with inlining and target_feature. For the static #[target_feature], the inline rules are just a hard-error version of the compatibility lint, which is great! (I hope in a future epoch the compatibility lint can be made a hard error.). But for cfg!(target_feature), we have some very interesting things going on:

1. Portability per node. For a fully analyzed cfg!(target_feature) we can look at the portability per mode of the CFG instead of per item. We'd probably want a difference syntax but the short story is that different branches of an if cfg!(...) have statically known portability, the dynamism is just the choice of edge. For cfg_feature_enabled! it's just the same except no branches must be pruned in the end.
2. Selective hygiene: A function must always be statically safe to call/inline if it is safe at all, but we "break" normal lexical rules to allow "improving" the interpretation of the formula. This can be formalized as passing in an exact portability as a static-parameter to be monomorphized.

More broadly, the concept of compiling for multiple variations of a platforms neatly extends into compiling for multiple platforms. #[target_feature] is just #[cfg(target_feature ...)] where the target_feature is neither true nor false in all current platforms. There is very little conceptual overhead in extending this to interpreting arbitrary cfg formulae over arbitrary sets of "active" (concurrently targeted) models. Given that one general system of "delayed cfg resolution" can implement both features, I'd strongly consider planning that.

As an aside, I guess we might as well track over areas of overlap between these two features in this issue too.

CC @gnzlbg

[gnzlbg]

Sounds good to me.

For cfg_feature_enabled! it's just the same except no branches must be pruned in the end.

So right now using cfg_feature_enabled! requires the user to open an unsafe { } block to call target-feature functions even though, as you mention, whether these are safe to call (as long as the "unsafety" stems only from #[target_feature]) is statically known:

fn foo() {
    if is_x86_feature_detected!("avx") { 
       unsafe { avx_function() } // unsafe REQUIRED
    } else {
        fallback_function()
    }
}

The unsafe block after RFC2396 (target_feature 1.1) would still be required above, because foo does not enable the avx feature.

Removing the need for the unsafe keyword in these cases would be an incremental step over RFC2396, but for this to be sound it would need to be an error and not a lint:

fn foo() {
    if is_x86_feature_detected!("avx") { 
       avx_function() // OK and sound
    } else if is_x86_feature_detected("sse4.2") {
       avx_function()  // ERROR: 
      // ^ call to AVX function from non-AVX context is unsafe
    } else if is_x86_feature_detected("sse3") {
       unsafe { avx_function() }  // OK, but portability warning 
    } else {
        fallback_function()
    }
}

I would suppose that the intent of the portability lint is just to provide portability warnings, which I think is a good first step. In particular, those macros expand to a bool, and I doubt we can change that.

One thing we could do, would be to expand those macros to something like:

fn foo() {
    if is_x86_feature_detected!("avx") { 
       avx_function() // OK and sound
    } else {
        fallback_function()
    }
}

fn foo_expanded() {
    if std::arch::__detect(...) #[target_feature("avx")] { 
       avx_function() // OK and sound
    } else {
        fallback_function()
    }
}

Or similar that enables the feature statically for a block. I don't think we can do this change in a backwards compatible way because code like let x = is_x86_feature_detected!("avx") || is_x86_feature_detected!("sse3") is perfectly valid code. But one thing we could do in a backwards compatible way is:

• allow #[target_feature] on blocks
• add a match macro that uses that and expands like this:

fn foo() {
    feature_match!() { 
       "avx" => avx_function(),
       _ =>  fallback_function(),
    }
}

fn foo_expanded() {
    if is_x86_feature_detected("avx") {
       #[target_feature(enable = "avx")]  {
            avx_function()  // OK: can be statically verified
       }
    } else {
        fallback_function(),
    }
}

So maybe we could try to pursue a target_feature 1.2 RFC after RFC2396 gets merged that extends the language to allow #[target_feature] to be used on code blocks. With that language change, the feature_match!() macro can be written in a library (although we probably want to expose it in std, which would allow us to implement it as a proc macro in rustc). This should play nicely with the portability lint and I can imagine us extending the feature_match!() macro to support more complex grammars without having to change the language any further:

fn foo() {
    feature_match!() { 
       "avx" | "avx2" => avx_function(),
       _ =>  fallback_function(),
    }
}

fn foo_expanded() {
    if is_x86_feature_detected("avx") {
       #[cfg_attr(not(cfg(feature = "avx2")), target_feature(enable = "avx")]
       #[cfg_attr(not(cfg(feature = "avx")), target_feature(enable = "avx2")] 
       {
            avx_function()  // OK: can be statically verified
       }
    } else {
        fallback_function(),
    }
}

and similar things.

[Ericson2314]

feature_match or any another macro that includes the branching sounds good for exactly the reason you say: the boolean condition and the asserting of target features in each branch are only unsafely connected, so we must abstract over. And yeah I was just implicitly thinking ahead to the portability lint raising errors not warnings, as seem to always accidentally do, haha.

[gnzlbg]

I've mentioned here: https://github.com/rust-lang/rust/issues/53069#issuecomment-410619469 an example that would be hard to check. That is:

fn foo(x: bool) {  // SSE2
    avx(); // WARNING
    if is_x86_feature_detected!("avx") {
        avx(); // OK (NO WARNING)
    }
    let b = is_x86_feature_detected!("avx");
    if b { avx(); } // OK (NO WARNING)
    if x { avx() } // ??? MIGHT BE OK
}

The problems I see are that here:

    if x { avx() } // ??? MIGHT BE OK

the x: bool doesn't convey anything about what it actually means. If it were x: Feature::AVX or similar we would know whether it is safe or unsafe to invoke avx() in that last statement.

Also, here:

    let b = is_x86_feature_detected!("avx");
    if b { avx(); } // OK (NO WARNING)

we need to do a fairly complex analysis to determine that this piece of code is "ok".

We would probably need to introduce a newer API that returns "better types" to be able to improve analysis quality for these cases. We just have to think that API through and make sure that it works correctly with the portability lint.

[Ericson2314]

Yeah any old bool will not do. Maybe if we #![cfg(..)] different variants of an enum that could allow those variants to function as proofs. (Think how ! proves the control flow path is unreachable.)

[lygstate]

fn foo(x: bool) {  // SSE2
    avx(); // WARNING
#    can these can be deteceted at the very begging?
# So this just expand to a variable.
    if is_x86_feature_detected!("avx") {
        avx(); // OK (NO WARNING)
    }
    let b = is_x86_feature_detected!("avx");
    if b { avx(); } // OK (NO WARNING)
    if x { avx() } // ??? MIGHT BE OK
}