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

Fix handling of large branch values in cranelift_frontend::Switch #8422

Merged
merged 1 commit into from
Apr 21, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 44 additions & 2 deletions cranelift/frontend/src/switch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,11 @@ impl Switch {
}

fn icmp_imm_u128(bx: &mut FunctionBuilder, cond: IntCC, x: Value, y: u128) -> Value {
if let Ok(index) = u64::try_from(y) {
bx.ins().icmp_imm(cond, x, index as i64)
if bx.func.dfg.value_type(x) != types::I128 {
assert!(u64::try_from(y).is_ok());
bx.ins().icmp_imm(cond, x, y as i64)
Comment on lines +293 to +294
Copy link
Member

Choose a reason for hiding this comment

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

This assert/conversion combination is a little surprising. If y converts to u64, doesn't that mean that there's a possibility that it would produce a negative i64 value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A negative i64 here is fine as the value on which we are operating is at most a 64bit integer and thus no implicit sign extension of the immediate will be done by icmp_imm.

} else if let Ok(index) = i64::try_from(y) {
bx.ins().icmp_imm(cond, x, index)
} else {
let (lsb, msb) = (y as u64, (y >> 64) as u64);
let lsb = bx.ins().iconst(types::I64, lsb as i64);
Expand Down Expand Up @@ -651,4 +654,43 @@ block4:
br_table v3, block3, [block2, block1]"
);
}

#[test]
fn switch_128bit_max_u64() {
let mut func = Function::new();
let mut func_ctx = FunctionBuilderContext::new();
{
let mut bx = FunctionBuilder::new(&mut func, &mut func_ctx);
let block0 = bx.create_block();
bx.switch_to_block(block0);
let val = bx.ins().iconst(types::I64, 0);
let val = bx.ins().uextend(types::I128, val);
let mut switch = Switch::new();
let block1 = bx.create_block();
switch.set_entry(u64::MAX.into(), block1);
let block2 = bx.create_block();
switch.set_entry(0, block2);
let block3 = bx.create_block();
switch.emit(&mut bx, val, block3);
}
let func = func
.to_string()
.trim_start_matches("function u0:0() fast {\n")
.trim_end_matches("\n}\n")
.to_string();
assert_eq_output!(
func,
"block0:
v0 = iconst.i64 0
v1 = uextend.i128 v0 ; v0 = 0
v2 = iconst.i64 -1
v3 = iconst.i64 0
v4 = iconcat v2, v3 ; v2 = -1, v3 = 0
v5 = icmp eq v1, v4
brif v5, block1, block4

block4:
brif.i128 v1, block3, block2"
);
}
}