Rollup of 5 pull requests

[Dylan-DPC]

Successful merges:

• Lower let-else in MIR #98574 (Lower let-else in MIR)
• UnsafeCell blocks niches inside its nested type from being available outside #99011 (UnsafeCell blocks niches inside its nested type from being available outside)
• diagnostics: error messages when struct literals fail to parse #99030 (diagnostics: error messages when struct literals fail to parse)
• Keep unstable target features for asm feature checking #99155 (Keep unstable target features for asm feature checking)
• Refactor: remove an unnecessary span_to_snippet #99199 (Refactor: remove an unnecessary span_to_snippet)

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

[Dylan-DPC]

@bors r+ rollup=never p=5

[bors]

📌 Commit 3933b2b has been approved by Dylan-DPC

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3933b2b with merge c80dde4...

[bors]

☀️ Test successful - checks-actions
Approved by: Dylan-DPC
Pushing c80dde4 to master...

[rust-timer]

Finished benchmarking commit (c80dde4): comparison url.

Instruction count

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: 😿 relevant regressions found

	mean1	max	count2
Regressions 😿 (primary)	2.6%	4.5%	7
Regressions 😿 (secondary)	1.6%	4.1%	55
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.6%	4.5%	7

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regressions found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.5%	4.8%	4
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	3.6%	4.4%	2
Regressions 😿 (secondary)	3.5%	3.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.6%	4.4%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[rylev]

@Amanieu it appears that maybe this regression is coming from #99155. Here's the top results from running a cachgrind diff:

1,399,189  ???:SetImpliedBits(llvm::FeatureBitset&, llvm::FeatureBitset const&, llvm::ArrayRef<llvm::SubtargetFeatureKV>)
   34,776  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/strcmp.S:strcmp
   24,034  ???:llvm::SubtargetFeatureKV const* Find<llvm::SubtargetFeatureKV>(llvm::StringRef, llvm::ArrayRef<llvm::SubtargetFeatureKV>)
   21,150  ???:llvm::X86_MC::initLLVMToSEHAndCVRegMapping(llvm::MCRegisterInfo*)
   20,919  ???:llvm::DenseMap<llvm::MCRegister, int, llvm::DenseMapInfo<llvm::MCRegister, void>, llvm::detail::DenseMapPair<llvm::MCRegister, int> >::grow(unsigned int)
   18,833  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S:__memcmp_avx2_movbe
   16,540  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/strlen-avx2.S:__strlen_avx2
   -5,975  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_middle::ty::subst::SubstFolder>

Notice that the top result is in LLVM for feature detection which would make sense perhaps if LLVMRustHasFeature is being called more. Thoughts?

[Amanieu]

This is quite surprising, I wasn't expecting this code to be hot. It's only called at startup, and even then only twice instead of once. I will see if I can change it to only query the LLVM features once.

[Amanieu]

It is quite concerning that a whole 5% of the compilation time of "hello world" is just querying LLVM for the list of target features. Perhaps we should consider moving the feature selection logic to rustc in the future.

[rylev]

That is surprising. Should we try to capture this in an issue? This is definitely a perf relevant piece of info that would be good to move out of this PR.

[nnethercote]

I looked at this SetImpliedBits stuff recently. AFAICT, the LLVM API is designed so you can query lots of feature bits at once, via a bitset, and you just get a boolean result indicating if all the feature bits are enabled. But rustc wants to query feature bits one at a time, and x86-64 has lots of features, so SetImpliedBits ends up being called 50+ times. It doesn't sound expensive, but helloworld is so fast to compile that it ends up being a non-trivial fraction of execution time.

I couldn't see how rustc could do better given the LLVM API, but I could easily have overlooked something.

[nnethercote]

Here are the notes I wrote down at the time:

Hard to improve. On x86-64 we query ~50 target feature flags, for things like SSE*, AVX*, etc. This is within target_features in compiler/rustc_codegen_llvm/src/llvm_util.rs. We check one flag at a time because the LLVM interface makes it hard to do otherwise, and LLVM is moderately slow to check each one. Even though it’s a significant fraction of execution time for small programs, the absolute time is low, so doesn’t seem worth any further effort.