Replace Copy/Clone compiler magic on arrays with library impls by bstrie · Pull...

Copy link

Contributor

rust-highfive commented on Jun 5

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been hidden.

tmiasko commented on Jun 7

The code building the clone shim for array can be removed now as well, right?

bstrie commented on Jun 12

I've addressed the feedback and this is ready for review.

bors commented on Aug 18

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

wesleywiser commented on Sep 2

Hi @bstrie! Would you mind rebasing this PR and resolving the merge conflict so we can do a perf run? Assuming that does not reveal any issues, I think we're about ready to merge this.

JohnCSimon commented on Sep 20

Ping from triage:
@bstrie can you please resolve the merge conflicts?

joshtriplett commented on Oct 4

This seems reasonable to me, once the merge conflicts are resolved and a perf run comes back without regressions.

bors commented 14 days ago

Try build successful - checks-actions
Build commit: 78d6575 (78d657592a19e656b8e8819fea9cb5eca34149f0)

rust-timer commented 14 days ago

rust-timer commented 13 days ago

Finished benchmarking commit (78d6575): comparison url.

Summary: This change led to moderate relevant mixed results in compiler performance.

• Moderate improvement in instruction counts (up to -1.2% on incr-unchanged builds of deeply-nested)
• Moderate regression in instruction counts (up to 1.4% on full builds of cranelift-codegen)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

jackh726 commented 13 days ago •

That looks better...mostly a bit more time spent in LLVM (not completely unexpected). Now this is even a slight improvement in a couple benchmarks.

With the latest results, the balance has shifted away from an intrinsic or some other magic being a good idea, imo.

I think performance results are definitely closer to being acceptable here, I'm OK with not bothering with trying out the shim approach or hand-coding it to generate better IR.

a1phyr commented 13 days ago

Maybe we could write a simple specialized function for clone that would be easier for LLVM to optimize on release and would require less codegen on debug (reusing the code for Guard):

fn clone(&self) -> Self {
    let mut array = MaybeUninit::uninit_array::<N>();
    let mut guard = Guard { array_mut: &mut array, initialized: 0 };

    for i in 0..N {
        let item = src[i].clone();
        guard.array_mut[i].write(item);
        guard.initialized = i;

    }

    mem::forget(guard);
    unsafe { MaybeUninit::array_assume_init(array) }
}

jackh726 commented 13 days ago

Maybe we could write a simple specialized function for clone that would be easier for LLVM to optimize on release and would require less codegen on debug (reusing the code for Guard):

Imo, i think this would better be left as followup.

jackh726 commented 13 days ago

Okay, given previous review from @joshtriplett, previous discussion during a compiler meeting, and perf review by @Mark-Simulacrum, and my own review of compiler/traits changes, I'm going to go ahead and @bors r+

bors commented 13 days ago

Commit 61b1394 has been approved by jackh726

bors commented 13 days ago

Testing commit 61b1394 with merge fc59b7e...

rust-log-analyzer commented 13 days ago

The job x86_64-msvc-cargo failed! Check out the build log: (web) (plain)

bors commented 13 days ago

Test failed - checks-actions

jackh726 commented 13 days ago

@bors retry

bors commented 13 days ago

Testing commit 61b1394 with merge d608229...

bors commented 13 days ago

Test successful - checks-actions
Approved by: jackh726
Pushing d608229 to master...

rust-timer commented 13 days ago

Finished benchmarking commit (d608229): comparison url.

Summary: This change led to moderate relevant mixed results in compiler performance.

• Moderate improvement in instruction counts (up to -1.2% on incr-unchanged builds of deeply-nested)
• Moderate regression in instruction counts (up to 1.0% on full builds of cranelift-codegen)

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