Resurrect #70477: "Use the niche optimization if other variant are small enough"

[erikdesjardins]

Fixes #46213, fixes #66029.

r? @eddyb

[erikdesjardins]

This needs a perf run, since results in the original PR were negative.

[ogoffart]

Last time i had a look at this issue, the benchmark showed performances regressions.
As explained here: #70477 (comment) , my hypotheses is that this change caused a runtime regression because the generated MIR code for match on enum with niche was not as optimal as it could be.

[bors]

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

[Dylan-DPC-zz]

let's do a perf run first:

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit ef282bb1cd28a6a4ce0b8dda66dc4a3250930a77 with merge d85bea00451112d262ee8d42afe2819e1153be8e...

[bors]

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

[rust-timer]

Queued d85bea00451112d262ee8d42afe2819e1153be8e with parent 10ef7f9, future comparison URL.

[rust-timer]

Finished benchmarking try commit (d85bea00451112d262ee8d42afe2819e1153be8e): 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

[erikdesjardins]

Perf is about the same as the original PR, regressions up to 4-5% on a lot of benchmarks.

[erikdesjardins]

Pushed another commit to disable the optimization and just reorder fields (tests will fail but the build should pass), let's do another perf run to see if that's the problem.

[Dylan-DPC-zz]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit d5ef4b6 with merge 4cdfe129f8f8acf6532f8252997ece91030dd135...

[bors]

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

[rust-timer]

Queued 4cdfe129f8f8acf6532f8252997ece91030dd135 with parent ef663a8, future comparison URL.

[rust-timer]

Finished benchmarking try commit (4cdfe129f8f8acf6532f8252997ece91030dd135): 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

[ogoffart]

The result seems to indicate that the problem is not caused by the re-ordering if the field and the change in the padding.
This seems to confirm what i wrote #70477 (comment) that the problem is because match when using the niche optimization is slower. It could either be slower for LLVM to compile, or the resulting code might be slower as well as there are more branches.

Either way, we could verify this hypotheses if we try to disable completely the niche optimization optimization (or only for type bigger than 8 bytes or so) and see if that improve the compile time performance.

I guess the fix would be to improve the code generation for the match.
I foresee a quite big improvement (even without the niche optimization) if doing what i try to explain in #70477 (comment)

In the perform_test function, we should not create a TeminatorKind::SwitchInt over Rvalue::Discriminant because the getting the discriminant will do a few branching and subtractions, before doing the next switch.

rust/compiler/rustc_mir_build/src/build/matches/test.rs

Lines 202 to 213 in be38081

	self.cfg.push_assign(block, source_info, discr, Rvalue::Discriminant(place));
	assert_eq!(values.len() + 1, targets.len());
	self.cfg.terminate(
	block,
	source_info,
	TerminatorKind::SwitchInt {
	discr: Operand::Move(discr),
	switch_ty: discr_ty,
	values: From::from(values),
	targets,
	},
	);

Instead, there should probably be a new TerminatorKind::Switch which maps the TestKind::Switch and keep the information about the variant, So codegen has more information to produce optimal code.

Currently, codegen generates something that looks like this (pseudo_code):

// this condition produces quite some instruction that llvm then need to try optimize. 
let discriminent = if (niche in [begin...end]) { niche - offset } else { data_variant };
match(discriminent) {
   data_variant => ...,
   other_variant1 => ...
   other_variant2 => ...
}

Instead, the codegen should do this:

match(niche) {
   other_variant1+offset => ...
   other_variant2+offset => ...
    _ => ...   // (for the data_variant)
   // begin..end  =>  ...    // alternative if the switch is not exhaustive
}

[ogoffart]

#77816 did a performence test to disable the niche optimization, showing small performence improvements. So I would conclude that the problem is indeed that the code generated for the niche enum is not optimal, and we should improve it before enabling the niche optimization in more cases.

[Dylan-DPC-zz]

@erikdesjardins any updates? (on the CI failure)

[erikdesjardins]

This can't be merged in its current state anyways, it's too much of a perf regression. It's likely that a codegen change (as described in #75866 (comment)) will be necessary in order to land this. I won't have time to look into that personally in the near future, so I'll close this for now.