Implement a first version of RFC 3525: struct target features

[veluca93]

This PR is an attempt at implementing rust-lang/rfcs#3525, behind a feature gate struct_target_features.

There's obviously a few tasks that ought to be done before this is merged; in no particular order:

• add proper error messages
• add tests
• create a tracking issue for the RFC
• properly serialize/deserialize the new target_features field in rmeta (assuming I even understood that correctly :-))

That said, as I am definitely not a rustc expert, I'd like to get some early feedback on the overall approach before fixing those things (and perhaps some pointers for rmeta...), hence this early PR :-)

Here's an example piece of code that I have been using for testing - with the new code, the calls to intrinsics get correctly inlined:

#![feature(struct_target_features)]

use std::arch::x86_64::*;

/*
// fails to compile
#[target_feature(enable = "avx")]
struct Invalid(u32);
*/

#[target_feature(enable = "avx")]
struct Avx {}

#[target_feature(enable = "sse")]
struct Sse();

/*
// fails to compile
extern "C" fn bad_fun(_: Avx) {}
*/

/*
// fails to compile
#[inline(always)]
fn inline_fun(_: Avx) {}
*/

trait Simd {
    fn do_something(&self);
}

impl Simd for Avx {
    fn do_something(&self) {
        unsafe {
            println!("{:?}", _mm256_setzero_ps());
        }
    }
}

impl Simd for Sse {
    fn do_something(&self) {
        unsafe {
            println!("{:?}", _mm_setzero_ps());
        }
    }
}

struct WithAvx {
    #[allow(dead_code)]
    avx: Avx,
}

impl Simd for WithAvx {
    fn do_something(&self) {
        unsafe {
            println!("{:?}", _mm256_setzero_ps());
        }
    }
}

#[inline(never)]
fn dosomething<S: Simd>(simd: &S) {
    simd.do_something();
}

fn main() {
    /*
    // fails to compile
    Avx {};
    */

    if is_x86_feature_detected!("avx") {
        let avx = unsafe { Avx {} };
        dosomething(&avx);
        dosomething(&WithAvx { avx });
    }
    if is_x86_feature_detected!("sse") {
        dosomething(&unsafe { Sse {} })
    }
}

Tracking:

• Tracking Issue for struct_target_features (RFC 3525) #129107

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[jieyouxu]

Rerolling because this touches metadata encoding and codegen attributes that I'm not familiar with.

r? compiler

[tgross35]

I know it is early but just as a heads up, you will need to add tests, probably at tests/ui/target-feature. If specific LLVM output is expected for anything, it will need a check in tests/codegen too.

Also a test at tests/ui/feature-gates to make sure this builds with the #![feature(...)] but errors without it.

[veluca93]

I know it is early but just as a heads up, you will need to add tests, probably at tests/ui/target-feature. If specific LLVM output is expected for anything, it will need a check in tests/codegen too.

Also a test at tests/ui/feature-gates to make sure this builds with the #![feature(...)] but errors without it.

Yup, I will add these tests when the time comes :-) probably makes sense to test codegen too - the easiest way I can think of relies on inliner behaviour to some extent, which IMO is not great, but I am not sure there's a better option. I will ask for pointers later though ;-)

[bors]

☔ The latest upstream changes (presumably #128298) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

[BoxyUwU]

Thanks for patience 😅

[bors]

⌛ Testing commit 7eb4cfe with merge acb4e8b...

[bors]

☀️ Test successful - checks-actions
Approved by: BoxyUwU
Pushing acb4e8b to master...

[rust-timer]

Finished benchmarking commit (acb4e8b): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 1.0%]	72
Regressions ❌ (secondary)	0.7%	[0.2%, 2.2%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 1.0%]	72

Max RSS (memory usage)

Results (primary 0.9%, secondary 2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.7%, 2.7%]	8
Regressions ❌ (secondary)	2.8%	[2.7%, 2.8%]	2
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[-1.1%, 2.7%]	9

Cycles

Results (primary -2.3%, secondary -2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-2.3%	[-2.7%, -2.0%]	2
Improvements ✅ (secondary)	-2.6%	[-2.9%, -2.2%]	12
All ❌✅ (primary)	-2.3%	[-2.7%, -2.0%]	2

Binary size

Results (primary 0.5%, secondary 1.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 1.6%]	122
Regressions ❌ (secondary)	1.2%	[0.0%, 2.4%]	28
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.0%, 1.6%]	122

Bootstrap: 753.9s -> 791.393s (4.97%)
Artifact size: 338.87 MiB -> 339.00 MiB (0.04%)

[nnethercote]

Perf results: significant instruction count and binary size regressions. (Also some walltime improvements, but they look like they might be reverting from a bad previous run and probably aren't meaningful.)

@veluca93: no performance runs were done before merging. Are these regressions expected? Any ideas on how to eliminate them?

[veluca93]

Perf results: significant instruction count and binary size regressions. (Also some walltime improvements, but they look like they might be reverting from a bad previous run and probably aren't meaningful.)

@veluca93: no performance runs were done before merging. Are these regressions expected? Any ideas on how to eliminate them?

On instruction count: I expected some compile time increase, although the extent of it is perhaps slightly surprising to me. There's a fixme about caching some intermediate values in the codebase that will probably mitigate the issue.

On binary size, OTOH, I am somewhat surprised - what does that benchmark measure exactly? This PR adds some fields to rmeta, but I am not sure if that should have such an effect...

[Kobzol]

For binary benchmarks, it measures the final binary size. For library benchmarks, it records the size of rlibs, so executable code + metadata.

[veluca93]

For binary benchmarks, it measures the final binary size. For library benchmarks, it records the size of rlibs, so executable code + metadata.

I see. Since this PR adds new metadata to ADTs, then the size increase for libraries is indeed expected. Results for binaries are within noise thresholds, so that checks out.

I am not sure if it is possible to mitigate this - the field I added is a Table<DefIndex, LazyArray<TargetFeature>>, with the LazyArray being empty in the vast majority of cases, so perhaps not populating the table at all when the array is empty might work... Will give this a try.

[pnkfelix]

Visiting for weekly rustc-perf triage

• this is being actively investigated by the PR author after getting feedback from @nnethercote
• not marking as triaged

[Kobzol]

We have a suspicion that this PR might have introduced an instability of perf. results that started appearing in recent days. Opened #129854 to see if a revert helps.