MIR-OPT: Less conservative EarlyOtherwiseBranch

[simonvandel]

It is not important that the types match in this optimization.
Instead, we can apply the optimization more liberally if the layouts match.

Locally when compiling stage 1 std, stage2 and stage 2 std this increases the times the optimization fires from 543 to 573.
A ~5% increase.

Also reorders checks so we check the cheapest first.

[rust-highfive]

r? @lcnr

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

[lcnr]

r? @oli-obk

[oli-obk]

Please explain this a bit more. It will be very surprising for future readers. It's not actually the layouts we care about, but the discriminant ids corresponding to the variants that we're looking for

[oli-obk]

We do have all the information at compile-time, maybe we should use it to figure out whether the two variants of different types that we're looking at have the same discriminant as returned by the Discriminant rvalue. That would be even less conservative

[simonvandel]

I'm a bit unsure what part of Layout (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/abi/struct.Layout.html) actually needs to match between the two types for us to say that this optimization is valid.

[oli-obk]

I don't think you need to check anything in the layout, you can check each type for the discriminant values directly:

rust/compiler/rustc_mir/src/interpret/operand.rs

Lines 657 to 666 in c2bc344

	let index = match *op.layout.ty.kind() {
	ty::Adt(adt, _) => {
	adt.discriminants(*self.tcx).find(|(_, var)| var.val == discr_bits)
	}
	ty::Generator(def_id, substs, _) => {
	let substs = substs.as_generator();
	substs
	.discriminants(def_id, *self.tcx)
	.find(|(_, var)| var.val == discr_bits)
	}

If you do this, please deduplicate your logic with the logic in const eval

[simonvandel]

Hmm, in what cases is it actually even necessary to check the type of the discriminants? We already check in find_discriminant_switch_pairing that the discriminant value of both switches match. See (of which the first check is redundant with the second)

rust/compiler/rustc_mir/src/transform/early_otherwise_branch.rs

Lines 258 to 282 in 9b77a6a

	// check that the value being matched on is the same. The
	if this_bb_discr_info.targets_with_values.iter().find(|x| x.1 == value).is_none() {
	trace!("NO: values being matched on are not the same");
	return None;
	}
	// only allow optimization if the left and right of the tuple being matched are the same variants.
	// so the following should not optimize
	// ```rust
	// let x: Option<()>;
	// let y: Option<()>;
	// match (x,y) {
	// (Some(_), None) => {},
	// _ => {}
	// }
	// ```
	// We check this by seeing that the value of the first discriminant is the only other discriminant value being used as a target in the second switch
	if !(this_bb_discr_info.targets_with_values.len() == 1
	&& this_bb_discr_info.targets_with_values[0].1 == value)
	{
	trace!(
	"NO: The second switch did not have only 1 target (besides otherwise) that had the same value as the value from the first switch that got us here"
	);
	return None;
	}

So this code https://godbolt.org/z/vW3j4h can still be optimized as both the lhs and lhs of the match are of the same discriminant value (1).

Do you see any problems with simply removing the layout check?

[oli-obk]

so... to recap. If the discriminants of the two variants being checked are the same and a very specific value, then we apply the optimization. This sounds to me like we can generalize the optimization even further, since almost the same thing happens with

if x == 42 && y == 42 {
    99
} else {
    69
}

I wasn't able to produce the same MIR as we get with the match, but if we can come up with user-code that produces the same code without using a match, then we should think about generalizing it.

Anyway, back to this PR. Yea, we need neither the redundant value check nor the type/layout equality check, as these don't actually prevent miscompilations as far as I can tell

[simonvandel]

For this PR: The newest commit removes the layout check and the redundant value check. So this should be ready for a review again.

For a future PR: Yeah true, your snippet does seem like something awfully lot like something this optimization could handle.

[simonvandel]

Seems like this miscompiles when compiling stage2. I'll see if I can find a repro

[simonvandel]

I (think) I fixed one miscompilation in the previous commit. However CI is not very happy. It fails quite early now compiling psm. It's probably another miscompilation which needs to be investigated. However, i'm not sure how to approach this. One idea was to binary search through optimization fuel until I find which function gets miscompiled and then dump the MIR for that function. But running RUSTFLAGS="-Z fuel=rustc=0" ./x.py build complains that i'm using a nightly Z flag on a beta compiler.

[bjorn3]

Use RUSTFLAGS_NOT_BOOTSTRAP.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[simonvandel]

I added back the block since that is definitely needed to disambiguate that we don't jump to the wrong case (and added some comments so I won't remove it again).

CI is still not pleased, so I'll stare some more at tests this week to figure out what is miscompiling.

[simonvandel]

It's still not clear to me what is miscompiling. I couldn't find any examples that are not handled correct, so at this point I don't know how to debug this. The stage2 compiler (or stage1 std) probably gets miscompiled which causes rustc to do funny stuff when running tests.

[tmiasko]

#[derive(Default)]
pub struct Matrix {
   rows: Vec<Option<u32>>,
}

impl Matrix {
   fn get(&self, row: usize) -> Option<&u32> {
       if let Some(Some(row)) = self.rows.get(row) { Some(row) } else { None }
   }
}

fn main() {
   let m  = Matrix::default();
   println!("{:?}", m.get(0));
}

[simonvandel]

Hi @tmiasko
Thanks! I'm curious, what was your process arriving at this repro?

[tmiasko]

@simonvandel I started by reviewing the code. There were two things that caught my attention:

1. Code commented with "assume this is the last statement of the block".
2. Hoisting the load of discriminant from one place to another.

I briefly tried to construct an example for the latter, but without possibility of constructing an arbitrary MIR failed to do so. Then I run rustc under gdb to find out where it crashed. The code looked like something that would be misoptimized, so by this point I was pretty sure I had a reproducer.

rust/compiler/rustc_index/src/bit_set.rs

Lines 1010 to 1012 in 738d4a7

	pub fn row(&self, row: R) -> Option<&HybridBitSet<C>> {
	if let Some(Some(row)) = self.rows.get(row) { Some(row) } else { None }
	}

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[simonvandel]

I'm closing this, since I won't be driving it further soon