Skip to content

Deduplicate operand creation between scalars, non-scalars and string patterns #136121

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Deduplicate operand creation between scalars, non-scalars and string patterns #136121

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9213c2
Deduplicate operand creation between scalars, non-scalars and string …
oli-obk Jan 27, 2025
e1e2e17
Use an operand instead of a place that is always turned into an operand
oli-obk Jan 27, 2025
d62f885
Edit the inputs to const == val check instead of duplicating logic
oli-obk Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 50 additions & 49 deletions compiler/rustc_mir_build/src/builder/matches/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,66 +141,71 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.cfg.terminate(block, self.source_info(match_start_span), terminator);
}

TestKind::Eq { value, ty } => {
TestKind::Eq { value, mut ty } => {
let tcx = self.tcx;
let success_block = target_block(TestBranch::Success);
let fail_block = target_block(TestBranch::Failure);
if let ty::Adt(def, _) = ty.kind()
&& tcx.is_lang_item(def.did(), LangItem::String)
{
if !tcx.features().string_deref_patterns() {
span_bug!(

let expect_ty = value.ty();
let expect = self.literal_operand(test.span, value);

let mut place = place;
let mut block = block;
match ty.kind() {
ty::Adt(def, _) if tcx.is_lang_item(def.did(), LangItem::String) => {
Comment on lines +154 to +155
Copy link
Contributor

Choose a reason for hiding this comment

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

Any stylistic reason for the match with a single arm over the let chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that I'll be adding a new variant immediately in a follow-up PR 😆

if !tcx.features().string_deref_patterns() {
span_bug!(
test.span,
"matching on `String` went through without enabling string_deref_patterns"
);
}
let re_erased = tcx.lifetimes.re_erased;
let ref_str_ty = Ty::new_imm_ref(tcx, re_erased, tcx.types.str_);
let ref_str = self.temp(ref_str_ty, test.span);
let eq_block = self.cfg.start_new_block();
// `let ref_str: &str = <String as Deref>::deref(&place);`
self.call_deref(
block,
eq_block,
place,
Mutability::Not,
ty,
ref_str,
test.span,
"matching on `String` went through without enabling string_deref_patterns"
);
// Since we generated a `ref_str = <String as Deref>::deref(&place) -> eq_block` terminator,
// we need to add all further statements to `eq_block`.
// Similarly, the normal test code should be generated for the `&str`, instead of the `String`.
block = eq_block;
Copy link
Member

Choose a reason for hiding this comment

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

Leave a comment about why we update these. i.e. something like we're lowering to X.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

place = ref_str;
ty = ref_str_ty;
}
let re_erased = tcx.lifetimes.re_erased;
let ref_str_ty = Ty::new_imm_ref(tcx, re_erased, tcx.types.str_);
let ref_str = self.temp(ref_str_ty, test.span);
let eq_block = self.cfg.start_new_block();
// `let ref_str: &str = <String as Deref>::deref(&place);`
self.call_deref(
block,
eq_block,
place,
Mutability::Not,
ty,
ref_str,
test.span,
);
self.non_scalar_compare(
eq_block,
success_block,
fail_block,
source_info,
value,
ref_str,
ref_str_ty,
);
} else if !ty.is_scalar() {
_ => {}
}

if !ty.is_scalar() {
// Use `PartialEq::eq` instead of `BinOp::Eq`
// (the binop can only handle primitives)
self.non_scalar_compare(
block,
success_block,
fail_block,
source_info,
value,
place,
expect,
expect_ty,
Operand::Copy(place),
ty,
);
} else {
assert_eq!(value.ty(), ty);
let expect = self.literal_operand(test.span, value);
let val = Operand::Copy(place);
assert_eq!(expect_ty, ty);
self.compare(
block,
success_block,
fail_block,
source_info,
BinOp::Eq,
expect,
val,
Operand::Copy(place),
);
}
}
Expand Down Expand Up @@ -371,12 +376,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
success_block: BasicBlock,
fail_block: BasicBlock,
source_info: SourceInfo,
value: Const<'tcx>,
mut val: Place<'tcx>,
mut expect: Operand<'tcx>,
expect_ty: Ty<'tcx>,
mut val: Operand<'tcx>,
mut ty: Ty<'tcx>,
) {
let mut expect = self.literal_operand(source_info.span, value);

// If we're using `b"..."` as a pattern, we need to insert an
// unsizing coercion, as the byte string has the type `&[u8; N]`.
//
Expand All @@ -391,7 +395,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
_ => None,
};
let opt_ref_ty = unsize(ty);
let opt_ref_test_ty = unsize(value.ty());
let opt_ref_test_ty = unsize(expect_ty);
match (opt_ref_ty, opt_ref_test_ty) {
// nothing to do, neither is an array
(None, None) => {}
Expand All @@ -410,11 +414,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
PointerCoercion::Unsize,
CoercionSource::Implicit,
),
Operand::Copy(val),
val,
ty,
),
);
val = temp;
val = Operand::Copy(temp);
}
if opt_ref_test_ty.is_some() {
let slice = self.temp(ty, source_info.span);
Expand Down Expand Up @@ -470,11 +474,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

const_: method,
})),
args: [Spanned { node: Operand::Copy(val), span: DUMMY_SP }, Spanned {
node: expect,
span: DUMMY_SP,
}]
.into(),
args: [Spanned { node: val, span: DUMMY_SP }, Spanned { node: expect, span: DUMMY_SP }]
.into(),
destination: eq_result,
target: Some(eq_block),
unwind: UnwindAction::Continue,
Expand Down
Loading