Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen #115236

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

scottmcm
Copy link
Member

Fixes #115212

It's unclear what makes this not work sometimes, since it often does work, so for now just disable the unusual cases. A future PR can consider doing something smarter, but this is an easy and safe tweak that we can do to resolve the regressions for now.

@scottmcm scottmcm added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 26, 2023
Comment on lines +44 to +51
#[no_mangle]
// CHECK-LABEL: @replace_short_array_4(
pub fn replace_short_array_4(r: &mut [u32; 4], v: [u32; 4]) -> [u32; 4] {
// CHECK-NOT: alloca
// CHECK: %[[R:.+]] = load <4 x i32>, ptr %r, align 4
// CHECK: store <4 x i32> %[[R]], ptr %result
// CHECK: %[[V:.+]] = load <4 x i32>, ptr %v, align 4
// CHECK: store <4 x i32> %[[V]], ptr %r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RIP.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me unless you want a specific review

@scottmcm
Copy link
Member Author

@bors r=compiler-errors rollup=never

@bors
Copy link
Contributor

bors commented Aug 26, 2023

📌 Commit 84e305d has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2023
@bors
Copy link
Contributor

bors commented Aug 26, 2023

⌛ Testing commit 84e305d with merge 766c0c0...

@Noratrieb
Copy link
Member

note that this is also what killed the MIPS backend so it seems reasonable to believe that LLVM backends are not good at this in general

@bors
Copy link
Contributor

bors commented Aug 26, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 766c0c0 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Aug 26, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 766c0c0 to master...

@bors bors added merged-by-bors This PR was explicitly merged by bors. labels Aug 26, 2023
@bors bors merged commit 766c0c0 into rust-lang:master Aug 26, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 26, 2023
@scottmcm scottmcm deleted the less-vector branch August 26, 2023 08:32
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (766c0c0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-2.8%, -2.8%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.911s -> 631.395s (-0.08%)
Artifact size: 315.57 MiB -> 315.55 MiB (-0.01%)

@scottmcm
Copy link
Member Author

scottmcm commented Aug 26, 2023

Beta nomination

This is to address the perf hit seen in #115212

It should be very safe, since it's only one line of product code, and all it does is disable #111999 in more cases, leaving those cases to use the same codepath as in 1.71.

(It would also be possible to disable 111999 entirely, but I've also seen cases like https://rust.godbolt.org/z/7s9ob85ja where the vector codegen helps avoid byte-by-byte movs, so this targets the known-bad case. As Nils mentions in #115236 (comment), non-power-of-two vectors also seem to be less well exercised in backends, so this avoids those problems by only using power-of-two vectors.)

EDIT: The original issue reporter confirmed in #115212 (comment) that this fixes it.

@wesleywiser wesleywiser added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Aug 31, 2023
@apiraino
Copy link
Contributor

apiraino commented Sep 1, 2023

Beta backport accepted as per compiler team on Zulip

Also in favor of a stable backport in case a 1.72.1 materializes (zulip comment).

@rustbot label +beta-accepted +stable-accepted

@rustbot rustbot added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Sep 1, 2023
@cuviper cuviper mentioned this pull request Sep 1, 2023
@cuviper cuviper modified the milestones: 1.74.0, 1.73.0 Sep 1, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 1, 2023
[beta] backports

- Contents of reachable statics is reachable rust-lang#115114
- Revert "Suggest using `Arc` on `!Send`/`!Sync` types" rust-lang#115311
- Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
- Do not forget to pass DWARF fragment information to LLVM. rust-lang#115139
- rustdoc: use unicode-aware checks for redundant explicit link fastpath rust-lang#115070

r? cuviper
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.73.0, 1.72.1 Sep 12, 2023
@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Sep 12, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2023
…Simulacrum

[stable] 1.72.1 release

This backports:

*  Remove assert that checks type equality rust-lang#115215
*  implied bounds: do not ICE on unconstrained region vars rust-lang#115559
*  rustdoc: correctly deal with self ty params when eliding default object lifetimes rust-lang#115276
*  Stop emitting non-power-of-two vectors in (non-portable-SIMD) codegen rust-lang#115236
*  Normalize before checking if local is freeze in deduced_param_attrs rust-lang#114948

Some cherry-picks required merge conflict resolution, we'll see if I got it right based on CI (rustdoc fix and LLVM codegen test).

r? `@Mark-Simulacrum`
mmastrac added a commit to denoland/deno that referenced this pull request Sep 20, 2023
> 1.72.1 resolves a few regressions introduced in 1.72.0:

> - [Partially revert codegen change, improving
codegen](rust-lang/rust#115236)
> - [rustdoc: Fix self ty params in objects with
lifetimes](rust-lang/rust#115276)
> - [Fix regression in compile
times](rust-lang/rust#114948)
> - Resolve some ICEs in the compiler:
>   - [#115215](rust-lang/rust#115215)
>   - [#115559](rust-lang/rust#115559)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nearly 50% performance regression when copying/moving small byte arrays between 1.71 and 1.72