Skip to content
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.

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

Use br instead of a conditional when switching on a constant boolean #120650

Merged
merged 1 commit into from
Feb 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 13 additions & 5 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
targets: &SwitchTargets,
) {
let discr = self.codegen_operand(bx, discr);
Copy link
Member

Choose a reason for hiding this comment

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

The other thing you could consider doing here is checking directly whether the discr is a constant, and if so using https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.SwitchTargets.html#method.target_for_value to decide the jump target without ever needing to talk to LLVM about it. Maybe that would help avoid worrying about the sign_ext and such, since you'll get a ScalarInt from the const that'll already have the u128 you need in it.

(And if somehow you get something that's not a scalar int, I think you can just ICE, since we obviously shouldn't be SwitchInting on a Scalar::Ptr.)

Copy link
Member

@compiler-errors compiler-errors Feb 6, 2024

Choose a reason for hiding this comment

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

target_for_value is already being called below--the problem here is that it takes a u128, and consts are stored in the compiler as scalars in the MIR or backend-specific values here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would checking if it's a constant then Const::try_to_bits be sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I took a closer look. I think it'd be

(I think the value you need is exactly the data: u128 in a ScalarInt, but there doesn't seem to be a way to get that out directly. But going this way means you stay in Rust, rather than making an LLVM value that won't be used.)

c-e is right that I missed that you're already using target_for_value. I just like having an excuse to mention it, I guess 🙃

Copy link
Member

Choose a reason for hiding this comment

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

try_to_bits should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into it, it looks like at this point we've already codegenned the i1 true, as if true creates a local which has true assigned to it (which wouldn't be safe to remove without checking for other uses of the local).
codegen_operand here then doesn't actually talk to LLVM, we go down to maybe_codegen_consume_direct which loads the i1 true reference directly from self.locals

Copy link
Contributor

Choose a reason for hiding this comment

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

which loads the i1 true reference directly from self.locals

While that is true for bool, other types will need to generate an llvm value and load from it.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again today, I still think we shouldn't need to codegen_operand here at all, rather match for a constant to do the special case before doing the LLVM stuff at all. (AKA not even have a .immediate() to get.)

let discr_value = discr.immediate();
let switch_ty = discr.layout.ty;
// If our discriminant is a constant we can branch directly
if let Some(const_discr) = bx.const_to_opt_u128(discr_value, false) {
let target = targets.target_for_value(const_discr);
bx.br(helper.llbb_with_cleanup(self, target));
return;
};

let mut target_iter = targets.iter();
if target_iter.len() == 1 {
// If there are two targets (one conditional, one fallback), emit `br` instead of
Expand All @@ -330,14 +338,14 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if switch_ty == bx.tcx().types.bool {
// Don't generate trivial icmps when switching on bool.
match test_value {
0 => bx.cond_br(discr.immediate(), llfalse, lltrue),
1 => bx.cond_br(discr.immediate(), lltrue, llfalse),
0 => bx.cond_br(discr_value, llfalse, lltrue),
1 => bx.cond_br(discr_value, lltrue, llfalse),
_ => bug!(),
}
} else {
let switch_llty = bx.immediate_backend_type(bx.layout_of(switch_ty));
let llval = bx.const_uint_big(switch_llty, test_value);
let cmp = bx.icmp(IntPredicate::IntEQ, discr.immediate(), llval);
let cmp = bx.icmp(IntPredicate::IntEQ, discr_value, llval);
bx.cond_br(cmp, lltrue, llfalse);
}
} else if self.cx.sess().opts.optimize == OptLevel::No
Expand All @@ -362,11 +370,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let ll2 = helper.llbb_with_cleanup(self, target2);
let switch_llty = bx.immediate_backend_type(bx.layout_of(switch_ty));
let llval = bx.const_uint_big(switch_llty, test_value1);
let cmp = bx.icmp(IntPredicate::IntEQ, discr.immediate(), llval);
let cmp = bx.icmp(IntPredicate::IntEQ, discr_value, llval);
bx.cond_br(cmp, ll1, ll2);
} else {
bx.switch(
discr.immediate(),
discr_value,
helper.llbb_with_cleanup(self, targets.otherwise()),
target_iter.map(|(value, target)| (value, helper.llbb_with_cleanup(self, target))),
);
Expand Down
67 changes: 67 additions & 0 deletions tests/codegen/constant-branch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
//@ compile-flags: -Zmir-opt-level=0 -C no-prepopulate-passes -Copt-level=0
// make sure that branching on a constant does not emit a conditional
// branch or a switch

#![crate_type = "lib"]

// CHECK-LABEL: @if_bool
#[no_mangle]
pub fn if_bool() {
// CHECK: br label %{{.+}}
_ = if true {
0
} else {
1
};

// CHECK: br label %{{.+}}
_ = if false {
0
} else {
1
};
}

// CHECK-LABEL: @if_constant_int_eq
#[no_mangle]
pub fn if_constant_int_eq() {
let val = 0;
// CHECK: br label %{{.+}}
_ = if val == 0 {
0
} else {
1
};

// CHECK: br label %{{.+}}
_ = if val == 1 {
0
} else {
1
};
}

// CHECK-LABEL: @if_constant_match
#[no_mangle]
pub fn if_constant_match() {
// CHECK: br label %{{.+}}
_ = match 1 {
1 => 2,
2 => 3,
_ => 4
};

// CHECK: br label %{{.+}}
_ = match 1 {
2 => 3,
_ => 4
};

// CHECK: br label %[[MINUS1:.+]]
_ = match -1 {
// CHECK: [[MINUS1]]:
// CHECK: store i32 1
-1 => 1,
_ => 0,
}
}
Loading