Optimize IntRange::from_pat, then shrink ParamEnv

[ecstatic-morse]

Resolves#77058.

r? @Mark-Simulacrum
cc @vandenheuvel

Looking at the output of perf report for #76244, the hot instructions seemed to be around the call to pat_constructor in IntRange::from_pat. I carried out an obvious optimization, but it actually made the instruction count higher (see #77075). However, it seems to have mitigated whatever was causing the pipeline stalls, so when combined with #76244, it's a net win.

As you can see below, the regression in #76244 seems to have originated from something measured by stalled-cycles-backend. I'll try to collect some finer-grained stats to see if I can isolate it. I wish I had a better idea of what was going on here. I'd like to prevent the regression from reappearing in the future due to small changes in unrelated code.

Current `master`:

 Performance counter stats for 'cargo +baseline-stage1 check':
2,275.67 msec task-clock:u # 0.998 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 49,826 page-faults:u # 0.022 M/sec 5,117,221,678 cycles:u # 2.249 GHz 299,655,943 stalled-cycles-frontend:u # 5.86% frontend cycles idle 2,284,213,395 stalled-cycles-backend:u # 44.64% backend cycles idle 8,051,871,959 instructions:u # 1.57 insn per cycle # 0.28 stalled cycles per insn
1,359,589,402 branches:u # 597.447 M/sec 7,359,347 branch-misses:u # 0.54% of all branches 2.281030026 seconds time elapsed
2.108197000 seconds user
0.164183000 seconds sys

Shrink `ParamEnv` without changing `IntRange::from_pat`:

 Performance counter stats for 'cargo +perf-stage1 check':
2,751.79 msec task-clock:u # 0.996 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 50,103 page-faults:u # 0.018 M/sec 6,260,590,019 cycles:u # 2.275 GHz 317,355,920 stalled-cycles-frontend:u # 5.07% frontend cycles idle 3,397,743,582 stalled-cycles-backend:u # 54.27% backend cycles idle 8,276,224,367 instructions:u # 1.32 insn per cycle # 0.41 stalled cycles per insn
1,370,453,386 branches:u # 498.023 M/sec 7,281,031 branch-misses:u # 0.53% of all branches 2.763265838 seconds time elapsed
2.544578000 seconds user
0.204548000 seconds sys

Shrink `ParamEnv` and change `IntRange::from_pat`:

 Performance counter stats for 'cargo +perf-stage1 check':
2,295.57 msec task-clock:u # 0.996 CPUs utilized 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 49,959 page-faults:u # 0.022 M/sec 5,151,407,066 cycles:u # 2.244 GHz 324,517,829 stalled-cycles-frontend:u # 6.30% frontend cycles idle 2,301,671,001 stalled-cycles-backend:u # 44.68% backend cycles idle 8,130,868,329 instructions:u # 1.58 insn per cycle # 0.28 stalled cycles per insn
1,356,618,512 branches:u # 590.972 M/sec 7,323,800 branch-misses:u # 0.54% of all branches 2.304509653 seconds time elapsed
2.128090000 seconds user
0.163909000 seconds sys

[ecstatic-morse]

@bors try
@rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit c4d8089 with merge baa01e8429de2c041f0095c01d4e8ca99c0b11a7...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: baa01e8429de2c041f0095c01d4e8ca99c0b11a7 (baa01e8429de2c041f0095c01d4e8ca99c0b11a7)

[rust-timer]

Queued baa01e8429de2c041f0095c01d4e8ca99c0b11a7 with parent 1ec980d, future comparison URL.

[rust-timer]

Finished benchmarking try commit (baa01e8429de2c041f0095c01d4e8ca99c0b11a7): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[ecstatic-morse]

(moved to summary)

[jackh726]

LGTM :) Super weird that this was the cause of the slowdown. (A large one at that!)

[vandenheuvel]

Very interesting. Please let me know if I'm understanding your comment correctly: combining these two issues results in this PR having a net gain, but this doesn't address the issue underlying #76913? So this would not close #77058?

[ecstatic-morse]

I'm not quite sure what you mean @vandenheuvel. I think I've gone as far as I care to. If you'd like to investigate further, feel free to keep #77058 open. Perhaps you can do even better.

Here's the "slow" assembly (ParamEnv shrunk with no other changes) in rustc_mir_build::thir::pattern::_match::PatStack::specialize_constructor() annotated via perf with the sum of StoreQueueRsrcStall and LoadQueueRsrcStall events (I'm on a Zen 2 Ryzen processor).

Excerpt from IntRange::from_pat (inlined)

0.00 → callq rustc_mir_build::thir::pattern::_match::pat_constructor0.00mov0xb0(%rsp),%cl0.02mov0xc0(%rsp),%r90.20mov0xb8(%rsp),%rbp0.01mov0xd0(%rsp),%r130.01mov0xc8(%rsp),%r80.01mov0xd8(%rsp),%al0.01mov0xd9(%rsp),%edx32.37mov %edx,0x40(%rsp)0.00mov0xdc(%rsp),%edx0.00mov %edx,0x43(%rsp)cmp$0x7,%cl ↓ jne 5a1 

Excerpt from (I think) IntRange::intersection (inlined)

 5a1: mov0x40(%rsp),%edx20.80mov0x43(%rsp),%esi0.03mov %esi,0x183(%rsp)mov %edx,0x180(%rsp)mov$0x1,%dlcmp$0x3,%cl ↓ jne 5d9 mov0x180(%rsp),%ecx0.01mov0x183(%rsp),%edx23.64mov %edx,0x23(%rsp)0.12mov %ecx,0x20(%rsp)cmp$0x2,%al sete %dl0.07 5d9: mov0x20(%rsp),%eax0.01mov0x23(%rsp),%ecx18.20mov %ecx,0xb3(%rsp)0.01mov %eax,0xb0(%rsp)test %dl,%dl0.00 ↓ jne1546mov %r14,0x18(%rsp)0.00mov0x218(%rsp),%rax0.00mov (%rax),%rdi0.01mov0x30(%rbx),%rax0.01mov0x8(%rbx),%r140.14mov0x10(%rbx),%rsimov0x20(%rbx),%rdxmov0x18(%rbx),%r10 movzbl (%rax),%ecxcmp$0x2,%rcx ↓ je629

We're repeatedly copying two words into local stack variables, then branching, then reading them again. This sequence has a lot of data hazards, which is clearly slowing down the pipeline significantly. I can't say why LLVM chose to generate code this way, or what causes the data hazards to disappear with my changes to IntRange::from_pat. You'd need someone with more LLVM expertise than I have. Perhaps building with debug-info-level=2 and stepping through it with a debugger would be helpful?

[simonvandel]

I think the tag was meant for @vandenheuvel

[ecstatic-morse]

I think the tag was meant for @vandenheuvel

Sorry!

[Mark-Simulacrum]

r=me unless you're waiting on something further, not quite sure.

[ecstatic-morse]

@bors r=Mark-Simulacrum

[bors]

📌 Commit c4d8089 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit c4d8089 with merge 48cab67...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 48cab67 to master...

[vandenheuvel]

@ecstatic-morse I don't have the expertise for further investigation either, unfortunately.

And why I asked whether #77058 would remain open: it seems that the root cause has not yet been found.

I wish I had a better idea of what was going on here. I'd like to prevent the regression from reappearing in the future due to small changes in unrelated code.

So to document that, it seems useful to keep #77058 open, perhaps after renaming it and adding another comment.

[ecstatic-morse]

I don't think there's anything actionable left in #77058. If you disagree, you're free to reopen it (you should have the requisite permissions).

[ecstatic-morse]

Final perf results are in. Looks to be an improvement everywhere except unicode-normaliaztion and the match stress test, as expected.