Skip to content

Commit 6377328

Browse files
alexcrichtonalexcrichton
committed
Revert "x64: Use 8-bit jumps in pseudo-insts (bytecodealliance#11271)"
This reverts commit 2fd1abf.
1 parent aec935f commit 6377328

File tree

99 files changed

+950
-995
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

99 files changed

+950
-995
lines changed

‎cranelift/codegen/src/isa/x64/inst/emit.rs

Lines changed: 17 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,6 @@ fn one_way_jmp(sink: &mut MachBuffer<Inst>, cc: CC, label: MachLabel) {
3939
debug_assert_eq!(sink.cur_offset(), cond_disp_off + 4);
4040
}
4141

42-
/// Like `one_way_jmp` but only used if the destination is <=127 bytes away.
43-
fn short_one_way_jmp(sink: &mut MachBuffer<Inst>, cc: CC, label: MachLabel) {
44-
let cond_start = sink.cur_offset();
45-
let cond_disp_off = cond_start + 1;
46-
sink.use_label_at_offset(cond_disp_off, label, LabelUse::JmpRel8);
47-
emit_short_jcc_no_offset(sink, cc);
48-
debug_assert_eq!(sink.cur_offset(), cond_disp_off + 1);
49-
}
50-
5142
/// Like `one_way_jmp` above emitting a conditional jump, but also using
5243
/// `MachBuffer::add_cond_branch`.
5344
fn cond_jmp(sink: &mut MachBuffer<Inst>, cc: CC, label: MachLabel) {
@@ -95,34 +86,6 @@ fn emit_jcc_no_offset(sink: &mut MachBuffer<Inst>, cc: CC) {
9586
});
9687
}
9788

98-
fn emit_short_jcc_no_offset(sink: &mut MachBuffer<Inst>, cc: CC) {
99-
// See `emit_jcc_no_offset` above for comments about subtle mismatches in
100-
// `CC` and `jcc` naming.
101-
let inst: AsmInst = match cc {
102-
CC::Z => asm::inst::je_d8::new(0).into(),
103-
CC::NZ => asm::inst::jne_d8::new(0).into(),
104-
CC::B => asm::inst::jb_d8::new(0).into(),
105-
CC::NB => asm::inst::jae_d8::new(0).into(),
106-
CC::BE => asm::inst::jbe_d8::new(0).into(),
107-
CC::NBE => asm::inst::ja_d8::new(0).into(),
108-
CC::L => asm::inst::jl_d8::new(0).into(),
109-
CC::LE => asm::inst::jle_d8::new(0).into(),
110-
CC::NL => asm::inst::jge_d8::new(0).into(),
111-
CC::NLE => asm::inst::jg_d8::new(0).into(),
112-
CC::O => asm::inst::jo_d8::new(0).into(),
113-
CC::NO => asm::inst::jno_d8::new(0).into(),
114-
CC::P => asm::inst::jp_d8::new(0).into(),
115-
CC::NP => asm::inst::jnp_d8::new(0).into(),
116-
CC::S => asm::inst::js_d8::new(0).into(),
117-
CC::NS => asm::inst::jns_d8::new(0).into(),
118-
};
119-
inst.encode(&mut external::AsmCodeSink {
120-
sink,
121-
incoming_arg_offset: 0,
122-
slot_offset: 0,
123-
});
124-
}
125-
12689
/// Emits an unconditional branch.
12790
fn uncond_jmp(sink: &mut MachBuffer<Inst>, label: MachLabel) {
12891
let uncond_start = sink.cur_offset();
@@ -290,7 +253,7 @@ pub(crate) fn emit(
290253
// go to the `idiv`.
291254
let inst = Inst::cmp_mi_sxb(size, *divisor, -1);
292255
inst.emit(sink, info, state);
293-
short_one_way_jmp(sink, CC::NZ, do_op);
256+
one_way_jmp(sink, CC::NZ, do_op);
294257

295258
// ... otherwise the divisor is -1 and the result is always 0. This
296259
// is written to the destination register which will be %rax for
@@ -380,7 +343,7 @@ pub(crate) fn emit(
380343
let next = sink.get_label();
381344

382345
// Jump if cc is *not* set.
383-
short_one_way_jmp(sink, cc.invert(), next);
346+
one_way_jmp(sink, cc.invert(), next);
384347
Inst::gen_move(dst.map(|r| r.to_reg()), consequent.to_reg(), *ty)
385348
.emit(sink, info, state);
386349

@@ -458,7 +421,7 @@ pub(crate) fn emit(
458421
// jne .loop_start
459422
// TODO: Encoding the conditional jump as a short jump
460423
// could save us us 4 bytes here.
461-
short_one_way_jmp(sink, CC::NZ, loop_start);
424+
one_way_jmp(sink, CC::NZ, loop_start);
462425

463426
// The regular prologue code is going to emit a `sub` after this, so we need to
464427
// reset the stack pointer
@@ -976,8 +939,8 @@ pub(crate) fn emit(
976939

977940
cmp_op.emit(sink, info, state);
978941

979-
short_one_way_jmp(sink, CC::NZ, do_min_max);
980-
short_one_way_jmp(sink, CC::P, propagate_nan);
942+
one_way_jmp(sink, CC::NZ, do_min_max);
943+
one_way_jmp(sink, CC::P, propagate_nan);
981944

982945
// Ordered and equal. The operands are bit-identical unless they are zero
983946
// and negative zero. These instructions merge the sign bits in that
@@ -994,7 +957,7 @@ pub(crate) fn emit(
994957
sink.bind_label(propagate_nan, state.ctrl_plane_mut());
995958
add_op.emit(sink, info, state);
996959

997-
short_one_way_jmp(sink, CC::P, done);
960+
one_way_jmp(sink, CC::P, done);
998961

999962
sink.bind_label(do_min_max, state.ctrl_plane_mut());
1000963
min_max_op.emit(sink, info, state);
@@ -1057,7 +1020,7 @@ pub(crate) fn emit(
10571020
// TODO use tst src, src here.
10581021
asm::inst::cmpq_mi_sxb::new(src, 0).emit(sink, info, state);
10591022

1060-
short_one_way_jmp(sink, CC::L, handle_negative);
1023+
one_way_jmp(sink, CC::L, handle_negative);
10611024

10621025
// Handle a positive int64, which is the "easy" case: a signed conversion will do the
10631026
// right thing.
@@ -1196,14 +1159,14 @@ pub(crate) fn emit(
11961159
let inst = Inst::cmp_mi_sxb(*dst_size, Gpr::unwrap_new(dst.to_reg()), 1);
11971160
inst.emit(sink, info, state);
11981161

1199-
short_one_way_jmp(sink, CC::NO, done); // no overflow => done
1162+
one_way_jmp(sink, CC::NO, done); // no overflow => done
12001163

12011164
// Check for NaN.
12021165
cmp_op.emit(sink, info, state);
12031166

12041167
if *is_saturating {
12051168
let not_nan = sink.get_label();
1206-
short_one_way_jmp(sink, CC::NP, not_nan); // go to not_nan if not a NaN
1169+
one_way_jmp(sink, CC::NP, not_nan); // go to not_nan if not a NaN
12071170

12081171
// For NaN, emit 0.
12091172
let inst: AsmInst = match *dst_size {
@@ -1231,7 +1194,7 @@ pub(crate) fn emit(
12311194
inst.emit(sink, info, state);
12321195

12331196
// Jump if >= to done.
1234-
short_one_way_jmp(sink, CC::NB, done);
1197+
one_way_jmp(sink, CC::NB, done);
12351198

12361199
// Otherwise, put INT_MAX.
12371200
if *dst_size == OperandSize::Size64 {
@@ -1424,12 +1387,12 @@ pub(crate) fn emit(
14241387
inst.emit(sink, info, state);
14251388

14261389
let handle_large = sink.get_label();
1427-
short_one_way_jmp(sink, CC::NB, handle_large); // jump to handle_large if src >= large_threshold
1390+
one_way_jmp(sink, CC::NB, handle_large); // jump to handle_large if src >= large_threshold
14281391

14291392
if *is_saturating {
14301393
// If not NaN jump over this 0-return, otherwise return 0
14311394
let not_nan = sink.get_label();
1432-
short_one_way_jmp(sink, CC::NP, not_nan);
1395+
one_way_jmp(sink, CC::NP, not_nan);
14331396

14341397
xor_op(dst, dst).emit(sink, info, state);
14351398

@@ -1450,7 +1413,7 @@ pub(crate) fn emit(
14501413
let inst = Inst::cmp_mi_sxb(*dst_size, Gpr::unwrap_new(dst.to_reg()), 0);
14511414
inst.emit(sink, info, state);
14521415

1453-
short_one_way_jmp(sink, CC::NL, done); // if dst >= 0, jump to done
1416+
one_way_jmp(sink, CC::NL, done); // if dst >= 0, jump to done
14541417

14551418
if *is_saturating {
14561419
// The input was "small" (< 2**(width -1)), so the only way to get an integer
@@ -1485,7 +1448,7 @@ pub(crate) fn emit(
14851448

14861449
if *is_saturating {
14871450
let next_is_large = sink.get_label();
1488-
short_one_way_jmp(sink, CC::NL, next_is_large); // if dst >= 0, jump to next_is_large
1451+
one_way_jmp(sink, CC::NL, next_is_large); // if dst >= 0, jump to next_is_large
14891452

14901453
// The input was "large" (>= 2**(width -1)), so the only way to get an integer
14911454
// overflow is because the input was too large: saturate to the max value.
@@ -1670,7 +1633,7 @@ pub(crate) fn emit(
16701633
inst.emit(sink, info, state);
16711634

16721635
// jnz again
1673-
short_one_way_jmp(sink, CC::NZ, again_label);
1636+
one_way_jmp(sink, CC::NZ, again_label);
16741637
}
16751638

16761639
Inst::Atomic128RmwSeq {
@@ -1790,7 +1753,7 @@ pub(crate) fn emit(
17901753
.emit(sink, info, state);
17911754

17921755
// jnz again
1793-
short_one_way_jmp(sink, CC::NZ, again_label);
1756+
one_way_jmp(sink, CC::NZ, again_label);
17941757
}
17951758

17961759
Inst::Atomic128XchgSeq {
@@ -1830,7 +1793,7 @@ pub(crate) fn emit(
18301793
.emit(sink, info, state);
18311794

18321795
// jnz again
1833-
short_one_way_jmp(sink, CC::NZ, again_label);
1796+
one_way_jmp(sink, CC::NZ, again_label);
18341797
}
18351798

18361799
Inst::ElfTlsGetAddr { symbol, dst } => {

‎cranelift/codegen/src/isa/x64/inst/emit_tests.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ fn test_x64_emit() {
292292
temp: w_r11.map(Gpr::unwrap_new),
293293
dst_old: w_rax.map(Gpr::unwrap_new),
294294
},
295-
"490FB6014989C34D0BDAF0450FB01975F3",
295+
"490FB6014989C34D0BDAF0450FB0190F85EFFFFFFF",
296296
"atomically { 8_bits_at_[%r9] Or= %r10; %rax = old_value_at_[%r9]; %r11, %rflags = trash }",
297297
));
298298
insns.push((
@@ -304,7 +304,7 @@ fn test_x64_emit() {
304304
temp: w_r11.map(Gpr::unwrap_new),
305305
dst_old: w_rax.map(Gpr::unwrap_new)
306306
},
307-
"490FB7014989C34D23DAF066450FB11975F2",
307+
"490FB7014989C34D23DAF066450FB1190F85EEFFFFFF",
308308
"atomically { 16_bits_at_[%r9] And= %r10; %rax = old_value_at_[%r9]; %r11, %rflags = trash }"
309309
));
310310
insns.push((
@@ -316,7 +316,7 @@ fn test_x64_emit() {
316316
temp: w_r11.map(Gpr::unwrap_new),
317317
dst_old: w_rax.map(Gpr::unwrap_new)
318318
},
319-
"418B014989C34D23DA49F7D3F0450FB11975F0",
319+
"418B014989C34D23DA49F7D3F0450FB1190F85ECFFFFFF",
320320
"atomically { 32_bits_at_[%r9] Nand= %r10; %rax = old_value_at_[%r9]; %r11, %rflags = trash }"
321321
));
322322
insns.push((
@@ -328,7 +328,7 @@ fn test_x64_emit() {
328328
temp: w_r11.map(Gpr::unwrap_new),
329329
dst_old: w_rax.map(Gpr::unwrap_new)
330330
},
331-
"418B014989C34539DA4D0F46DAF0450FB11975EF",
331+
"418B014989C34539DA4D0F46DAF0450FB1190F85EBFFFFFF",
332332
"atomically { 32_bits_at_[%r9] Umin= %r10; %rax = old_value_at_[%r9]; %r11, %rflags = trash }"
333333
));
334334
insns.push((
@@ -340,7 +340,7 @@ fn test_x64_emit() {
340340
temp: w_r11.map(Gpr::unwrap_new),
341341
dst_old: w_rax.map(Gpr::unwrap_new)
342342
},
343-
"498B014989C34D39DA4D0F4DDAF04D0FB11975EF",
343+
"498B014989C34D39DA4D0F4DDAF04D0FB1190F85EBFFFFFF",
344344
"atomically { 64_bits_at_[%r9] Smax= %r10; %rax = old_value_at_[%r9]; %r11, %rflags = trash }"
345345
));
346346

@@ -356,7 +356,7 @@ fn test_x64_emit() {
356356
dst_old_low: w_rax.map(Gpr::unwrap_new),
357357
dst_old_high: w_rdx.map(Gpr::unwrap_new),
358358
},
359-
"498B01498B51084889C34889D1490BDA490BCBF0490FC70975ED",
359+
"498B01498B51084889C34889D1490BDA490BCBF0490FC7090F85E9FFFFFF",
360360
"atomically { %rdx:%rax = 0(%r9); %rcx:%rbx = %rdx:%rax Or %r11:%r10; 0(%r9) = %rcx:%rbx }",
361361
));
362362
insns.push((
@@ -370,7 +370,7 @@ fn test_x64_emit() {
370370
dst_old_low: w_rax.map(Gpr::unwrap_new),
371371
dst_old_high: w_rdx.map(Gpr::unwrap_new),
372372
},
373-
"498B01498B51084889C34889D14923DA4923CBF0490FC70975ED",
373+
"498B01498B51084889C34889D14923DA4923CBF0490FC7090F85E9FFFFFF",
374374
"atomically { %rdx:%rax = 0(%r9); %rcx:%rbx = %rdx:%rax And %r11:%r10; 0(%r9) = %rcx:%rbx }"
375375
));
376376
insns.push((
@@ -384,7 +384,7 @@ fn test_x64_emit() {
384384
dst_old_low: w_rax.map(Gpr::unwrap_new),
385385
dst_old_high: w_rdx.map(Gpr::unwrap_new),
386386
},
387-
"498B01498B51084889C34889D14C39D3491BCB4889D1490F43DA490F43CBF0490FC70975E2",
387+
"498B01498B51084889C34889D14C39D3491BCB4889D1490F43DA490F43CBF0490FC7090F85DEFFFFFF",
388388
"atomically { %rdx:%rax = 0(%r9); %rcx:%rbx = %rdx:%rax Umin %r11:%r10; 0(%r9) = %rcx:%rbx }"
389389
));
390390
insns.push((
@@ -398,7 +398,7 @@ fn test_x64_emit() {
398398
dst_old_low: w_rax.map(Gpr::unwrap_new),
399399
dst_old_high: w_rdx.map(Gpr::unwrap_new),
400400
},
401-
"498B01498B51084889C34889D14903DA4913CBF0490FC70975ED",
401+
"498B01498B51084889C34889D14903DA4913CBF0490FC7090F85E9FFFFFF",
402402
"atomically { %rdx:%rax = 0(%r9); %rcx:%rbx = %rdx:%rax Add %r11:%r10; 0(%r9) = %rcx:%rbx }"
403403
));
404404
insns.push((
@@ -409,7 +409,7 @@ fn test_x64_emit() {
409409
dst_old_low: w_rax.map(Gpr::unwrap_new),
410410
dst_old_high: w_rdx.map(Gpr::unwrap_new),
411411
},
412-
"498B01498B5108F0490FC70975F9",
412+
"498B01498B5108F0490FC7090F85F5FFFFFF",
413413
"atomically { %rdx:%rax = 0(%r9); 0(%r9) = %rcx:%rbx }",
414414
));
415415

‎cranelift/codegen/src/isa/x64/inst/mod.rs

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,14 +1562,6 @@ pub enum LabelUse {
15621562
/// next instruction (so the size of the payload -- 4 bytes -- is subtracted from the payload).
15631563
JmpRel32,
15641564

1565-
/// An 8-bit offset from location of relocation itself, added to the
1566-
/// existing value at that location.
1567-
///
1568-
/// Used for control flow instructions which consider an offset from the
1569-
/// start of the next instruction (so the size of the payload -- 1 byte --
1570-
/// is subtracted from the payload).
1571-
JmpRel8,
1572-
15731565
/// A 32-bit offset from location of relocation itself, added to the existing value at that
15741566
/// location.
15751567
PCRel32,
@@ -1581,21 +1573,18 @@ impl MachInstLabelUse for LabelUse {
15811573
fn max_pos_range(self) -> CodeOffset {
15821574
match self {
15831575
LabelUse::JmpRel32 | LabelUse::PCRel32 => 0x7fff_ffff,
1584-
LabelUse::JmpRel8 => 0x7f,
15851576
}
15861577
}
15871578

15881579
fn max_neg_range(self) -> CodeOffset {
15891580
match self {
15901581
LabelUse::JmpRel32 | LabelUse::PCRel32 => 0x8000_0000,
1591-
LabelUse::JmpRel8 => 0x80,
15921582
}
15931583
}
15941584

15951585
fn patch_size(self) -> CodeOffset {
15961586
match self {
15971587
LabelUse::JmpRel32 | LabelUse::PCRel32 => 4,
1598-
LabelUse::JmpRel8 => 1,
15991588
}
16001589
}
16011590

@@ -1610,9 +1599,6 @@ impl MachInstLabelUse for LabelUse {
16101599
let value = pc_rel.wrapping_add(addend).wrapping_sub(4);
16111600
buffer.copy_from_slice(&value.to_le_bytes()[..]);
16121601
}
1613-
LabelUse::JmpRel8 => {
1614-
buffer[0] = buffer[0].wrapping_add(pc_rel as u8).wrapping_sub(1);
1615-
}
16161602
LabelUse::PCRel32 => {
16171603
let addend = u32::from_le_bytes([buffer[0], buffer[1], buffer[2], buffer[3]]);
16181604
let value = pc_rel.wrapping_add(addend);
@@ -1624,21 +1610,12 @@ impl MachInstLabelUse for LabelUse {
16241610
fn supports_veneer(self) -> bool {
16251611
match self {
16261612
LabelUse::JmpRel32 | LabelUse::PCRel32 => false,
1627-
1628-
// Technically this is possible to have a veneer because it can jump
1629-
// to a 32-bit jump which keeps going. That being said at this time
1630-
// this variant is only used in `emit.rs` for jumps that are already
1631-
// known to be short so it's a bug if we jump to a jump that's too
1632-
// far away. In the future if general-purpose basic-block
1633-
// terminators are switched to using short jumps to get promoted to
1634-
// a long jump then this may wish to change.
1635-
LabelUse::JmpRel8 => false,
16361613
}
16371614
}
16381615

16391616
fn veneer_size(self) -> CodeOffset {
16401617
match self {
1641-
LabelUse::JmpRel32 | LabelUse::PCRel32 | LabelUse::JmpRel8 => 0,
1618+
LabelUse::JmpRel32 | LabelUse::PCRel32 => 0,
16421619
}
16431620
}
16441621

@@ -1648,7 +1625,7 @@ impl MachInstLabelUse for LabelUse {
16481625

16491626
fn generate_veneer(self, _: &mut [u8], _: CodeOffset) -> (CodeOffset, LabelUse) {
16501627
match self {
1651-
LabelUse::JmpRel32 | LabelUse::PCRel32 | LabelUse::JmpRel8 => {
1628+
LabelUse::JmpRel32 | LabelUse::PCRel32 => {
16521629
panic!("Veneer not supported for JumpRel32 label-use.");
16531630
}
16541631
}

‎cranelift/filetests/filetests/isa/x64/cmp-mem-bug.clif

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,9 @@ block0(v0: f64, v1: i64):
8181
; movzbq %dil, %rax
8282
; ucomisd %xmm1, %xmm0
8383
; movdqa %xmm0, %xmm2
84-
; jnp 0x26
84+
; jnp 0x2a
8585
; movaps %xmm2, %xmm0
86-
; je 0x2b
86+
; je 0x33
8787
; movaps %xmm2, %xmm0
8888
; movq %rbp, %rsp
8989
; popq %rbp

0 commit comments

Comments
 (0)