T0b1/wasmtime
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

x64: handle tests of b1 values correctly (only LSB is defined).

Browse Source 
Previously, `select` and `brz`/`brnz` instructions, when given a `b1`
boolean argument, would test whether that boolean argument was nonzero,
rather than whether its LSB was nonzero. Since our invariant for mapping
CLIF state to machine state is that bits beyond the width of a value are
undefined, the proper lowering is to test only the LSB.

(aarch64 does not have the same issue because its `Extend` pseudoinst
already properly handles masking of b1 values when a zero-extend is
requested, as it is for select/brz/brnz.)

Found by Nathan Ringo on Zulip [1] (thanks!).

[1]
https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/bnot.20on.20b1s
This commit is contained in:
Chris Fallin
2021-01-04 16:42:24 -08:00
parent 2b325a1878
commit dbd2241b60
7 changed files with 271 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
cranelift/codegen/src/isa/x64/inst/args.rs
View File

@@ -397,6 +397,14 @@ impl fmt::Display for UnaryRmROpcode {
}
}
#[derive(Clone, Copy, PartialEq)]
pub enum CmpOpcode {
/// CMP instruction: compute `a - b` and set flags from result.
Cmp,
/// TEST instruction: compute `a & b` and set flags from result.
Test,
}
pub(crate) enum InstructionSet {
SSE,
SSE2,

45
cranelift/codegen/src/isa/x64/inst/emit.rs
View File

@@ -1311,7 +1311,13 @@ pub(crate) fn emit(
size,
src: src_e,
dst: reg_g,
opcode,
} => {
let is_cmp = match opcode {
CmpOpcode::Cmp => true,
CmpOpcode::Test => false,
};
let mut prefix = LegacyPrefixes::None;
if *size == 2 {
prefix = LegacyPrefixes::_66;
@@ -1342,16 +1348,26 @@ pub(crate) fn emit(
}
}
// Use the swapped operands encoding, to stay consistent with the output of
// Use the swapped operands encoding for CMP, to stay consistent with the output of
// gcc/llvm.
let opcode = if *size == 1 { 0x38 } else { 0x39 };
let opcode = match (*size, is_cmp) {
(1, true) => 0x38,
(_, true) => 0x39,
(1, false) => 0x84,
(_, false) => 0x85,
};
emit_std_reg_reg(sink, prefix, opcode, 1, *reg_e, *reg_g, rex);
}
RegMemImm::Mem { addr } => {
let addr = &addr.finalize(state, sink);
// Whereas here we revert to the "normal" G-E ordering.
let opcode = if *size == 1 { 0x3A } else { 0x3B };
// Whereas here we revert to the "normal" G-E ordering for CMP.
let opcode = match (*size, is_cmp) {
(1, true) => 0x3A,
(_, true) => 0x3B,
(1, false) => 0x84,
(_, false) => 0x85,
};
emit_std_reg_mem(sink, state, info, prefix, opcode, 1, *reg_g, addr, rex);
}
@@ -1361,16 +1377,25 @@ pub(crate) fn emit(
let use_imm8 = low8_will_sign_extend_to_32(*simm32);
// And also here we use the "normal" G-E ordering.
let opcode = if *size == 1 {
0x80
} else if use_imm8 {
0x83
let opcode = if is_cmp {
if *size == 1 {
0x80
} else if use_imm8 {
0x83
} else {
0x81
}
} else {
0x81
if *size == 1 {
0xF6
} else {
0xF7
}
};
let subopcode = if is_cmp { 7 } else { 0 };
let enc_g = int_reg_enc(*reg_g);
emit_std_enc_enc(sink, prefix, opcode, 1, 7 /*subopcode*/, enc_g, rex);
emit_std_enc_enc(sink, prefix, opcode, 1, subopcode, enc_g, rex);
emit_simm(sink, if use_imm8 { 1 } else { *size }, *simm32);
}
}

82
cranelift/codegen/src/isa/x64/inst/emit_tests.rs
View File

@@ -2648,6 +2648,88 @@ fn test_x64_emit() {
"cmpb %r13b, %r14b",
));
// ========================================================
// TestRmiR
insns.push((
Inst::test_rmi_r(8, RegMemImm::reg(r15), rdx),
"4C85FA",
"testq %r15, %rdx",
));
insns.push((
Inst::test_rmi_r(8, RegMemImm::mem(Amode::imm_reg(99, rdi)), rdx),
"48855763",
"testq 99(%rdi), %rdx",
));
insns.push((
Inst::test_rmi_r(8, RegMemImm::imm(76543210), rdx),
"48F7C2EAF48F04",
"testq $76543210, %rdx",
));
//
insns.push((
Inst::test_rmi_r(4, RegMemImm::reg(r15), rdx),
"4485FA",
"testl %r15d, %edx",
));
insns.push((
Inst::test_rmi_r(4, RegMemImm::mem(Amode::imm_reg(99, rdi)), rdx),
"855763",
"testl 99(%rdi), %edx",
));
insns.push((
Inst::test_rmi_r(4, RegMemImm::imm(76543210), rdx),
"F7C2EAF48F04",
"testl $76543210, %edx",
));
//
insns.push((
Inst::test_rmi_r(2, RegMemImm::reg(r15), rdx),
"664485FA",
"testw %r15w, %dx",
));
insns.push((
Inst::test_rmi_r(2, RegMemImm::mem(Amode::imm_reg(99, rdi)), rdx),
"66855763",
"testw 99(%rdi), %dx",
));
insns.push((
Inst::test_rmi_r(2, RegMemImm::imm(23210), rdx),
"66F7C2AA5A",
"testw $23210, %dx",
));
//
insns.push((
Inst::test_rmi_r(1, RegMemImm::reg(r15), rdx),
"4484FA",
"testb %r15b, %dl",
));
insns.push((
Inst::test_rmi_r(1, RegMemImm::mem(Amode::imm_reg(99, rdi)), rdx),
"845763",
"testb 99(%rdi), %dl",
));
insns.push((
Inst::test_rmi_r(1, RegMemImm::imm(70), rdx),
"F6C246",
"testb $70, %dl",
));
// Extra byte-cases (paranoia!) for test_rmi_r for first operand = R
insns.push((
Inst::test_rmi_r(1, RegMemImm::reg(rax), rbx),
"84C3",
"testb %al, %bl",
));
insns.push((
Inst::test_rmi_r(1, RegMemImm::reg(rcx), rsi),
"4084CE",
"testb %cl, %sil",
));
insns.push((
Inst::test_rmi_r(1, RegMemImm::reg(rcx), r10),
"4184CA",
"testb %cl, %r10b",
));
// ========================================================
// SetCC
insns.push((Inst::setcc(CC::O, w_rsi), "400F90C6", "seto %sil"));

52
cranelift/codegen/src/isa/x64/inst/mod.rs
View File

@@ -168,9 +168,10 @@ pub enum Inst {
dst: Writable<Reg>,
},
/// Integer comparisons/tests: cmp (b w l q) (reg addr imm) reg.
/// Integer comparisons/tests: cmp or test (b w l q) (reg addr imm) reg.
CmpRmiR {
size: u8, // 1, 2, 4 or 8
opcode: CmpOpcode,
src: RegMemImm,
dst: Reg,
},
@@ -913,8 +914,30 @@ impl Inst {
) -> Inst {
src.assert_regclass_is(RegClass::I64);
debug_assert!(size == 8 || size == 4 || size == 2 || size == 1);
debug_assert!(dst.get_class() == RegClass::I64);
Inst::CmpRmiR { size, src, dst }
debug_assert_eq!(dst.get_class(), RegClass::I64);
Inst::CmpRmiR {
size,
src,
dst,
opcode: CmpOpcode::Cmp,
}
}
/// Does a comparison of dst & src for operands of size `size`.
pub(crate) fn test_rmi_r(
size: u8, // 1, 2, 4 or 8
src: RegMemImm,
dst: Reg,
) -> Inst {
src.assert_regclass_is(RegClass::I64);
debug_assert!(size == 8 || size == 4 || size == 2 || size == 1);
debug_assert_eq!(dst.get_class(), RegClass::I64);
Inst::CmpRmiR {
size,
src,
dst,
opcode: CmpOpcode::Test,
}
}
pub(crate) fn trap(trap_code: TrapCode) -> Inst {
@@ -1597,12 +1620,23 @@ impl PrettyPrint for Inst {
dst.to_reg().show_rru(mb_rru)
),
Inst::CmpRmiR { size, src, dst } => format!(
"{} {}, {}",
ljustify2("cmp".to_string(), suffix_bwlq(*size)),
src.show_rru_sized(mb_rru, *size),
show_ireg_sized(*dst, mb_rru, *size)
),
Inst::CmpRmiR {
size,
src,
dst,
opcode,
} => {
let op = match opcode {
CmpOpcode::Cmp => "cmp",
CmpOpcode::Test => "test",
};
format!(
"{} {}, {}",
ljustify2(op.to_string(), suffix_bwlq(*size)),
src.show_rru_sized(mb_rru, *size),
show_ireg_sized(*dst, mb_rru, *size)
)
}
Inst::Setcc { cc, dst } => format!(
"{} {}",

2
cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs
View File

@@ -175,7 +175,7 @@ mod tests {
_ => panic!("expected unwind information"),
};
assert_eq!(format!("{:?}", fde), "FrameDescriptionEntry { address: Constant(4321), length: 23, lsda: None, instructions: [(1, CfaOffset(16)), (1, Offset(Register(6), -16)), (4, CfaRegister(Register(6))), (16, RememberState), (18, RestoreState)] }");
assert_eq!(format!("{:?}", fde), "FrameDescriptionEntry { address: Constant(4321), length: 22, lsda: None, instructions: [(1, CfaOffset(16)), (1, Offset(Register(6), -16)), (4, CfaRegister(Register(6))), (15, RememberState), (17, RestoreState)] }");
}
fn create_multi_return_function(call_conv: CallConv) -> Function {