Skip to content

Commit

Permalink
Merge #3415
Browse files Browse the repository at this point in the history 
3415: Fix singlepass for Aarch64 r=ptitSeb a=ptitSeb

# Description
Following debug of ticket #3343 , some issues in the singlepass were found.
In particular, some ADD/SUB opcode involving XSp with large value were not using the correct opcode, and `sub xsp, xsp, x17` were encoded as `neg xzr, x17` (a.k.a. `sub xzr, xzr, x17`). By forcing the dummy `UXTX` modifier, the correct opcode is selected.
Changed both SUB and ADD emiters.
Some alignement function specific to macOS ABI were also adjusted, and Local variable storage/retreival optimized a bit.


Co-authored-by: ptitSeb <[email protected]>
  • Loading branch information
@bors @ptitSeb
bors[bot] and ptitSeb authored Dec 9, 2022
2 parents ef8d2f6 + de03d35 commit 2d17aa9
Show file tree
Hide file tree
Showing 3 changed files with 173 additions and 56 deletions.
56 changes: 28 additions & 28 deletions lib/compiler-singlepass/src/emitter_arm64.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1303,6 +1303,12 @@ impl EmitterARM64 for Assembler {
let masked = 0xffff & (val >> offset);
if (masked << offset) == val {
dynasm!(self ; movz X(dst), masked as u32, LSL offset);
} else if val >> 16 == 0xffff_ffff_ffff {
let val: u16 = !((val & 0xffff) as u16);
dynasm!(self ; movn X(dst), val as u32);
} else if val >> 16 == 0xffff {
let val: u16 = !((val & 0xffff) as u16);
dynasm!(self ; movn W(dst), val as u32);
} else {
dynasm!(self ; movz W(dst), (val&0xffff) as u32);
let val = val >> 16;
Expand Down Expand Up @@ -1336,13 +1342,13 @@ impl EmitterARM64 for Assembler {
let src1 = src1.into_index() as u32;
let src2 = src2.into_index() as u32;
let dst = dst.into_index() as u32;
dynasm!(self ; add X(dst), X(src1), X(src2));
dynasm!(self ; add X(dst), X(src1), X(src2), UXTX);
}
(Size::S32, Location::GPR(src1), Location::GPR(src2), Location::GPR(dst)) => {
let src1 = src1.into_index() as u32;
let src2 = src2.into_index() as u32;
let dst = dst.into_index() as u32;
dynasm!(self ; add W(dst), W(src1), W(src2));
dynasm!(self ; add W(dst), W(src1), W(src2), UXTX);
}
(Size::S64, Location::GPR(src1), Location::Imm8(imm), Location::GPR(dst))
| (Size::S64, Location::Imm8(imm), Location::GPR(src1), Location::GPR(dst)) => {
Expand Down Expand Up @@ -1406,13 +1412,13 @@ impl EmitterARM64 for Assembler {
let src1 = src1.into_index() as u32;
let src2 = src2.into_index() as u32;
let dst = dst.into_index() as u32;
dynasm!(self ; sub X(dst), X(src1), X(src2));
dynasm!(self ; sub X(dst), X(src1), X(src2), UXTX);
}
(Size::S32, Location::GPR(src1), Location::GPR(src2), Location::GPR(dst)) => {
let src1 = src1.into_index() as u32;
let src2 = src2.into_index() as u32;
let dst = dst.into_index() as u32;
dynasm!(self ; sub W(dst), W(src1), W(src2));
dynasm!(self ; sub W(dst), W(src1), W(src2), UXTX);
}
(Size::S64, Location::GPR(src1), Location::Imm8(imm), Location::GPR(dst)) => {
let src1 = src1.into_index() as u32;
Expand Down Expand Up @@ -3257,24 +3263,17 @@ pub fn gen_std_trampoline_arm64(
#[allow(clippy::single_match)]
match calling_convention {
CallingConvention::AppleAarch64 => {
match sz {
Size::S8 => (),
Size::S16 => {
if caller_stack_offset & 1 != 0 {
caller_stack_offset = (caller_stack_offset + 1) & !1;
}
}
Size::S32 => {
if caller_stack_offset & 3 != 0 {
caller_stack_offset = (caller_stack_offset + 3) & !3;
}
}
Size::S64 => {
if caller_stack_offset & 7 != 0 {
caller_stack_offset = (caller_stack_offset + 7) & !7;
}
}
};
let sz = 1
<< match sz {
Size::S8 => 0,
Size::S16 => 1,
Size::S32 => 2,
Size::S64 => 3,
};
// align first
if sz > 1 && caller_stack_offset & (sz - 1) != 0 {
caller_stack_offset = (caller_stack_offset + (sz - 1)) & !(sz - 1);
}
}
_ => (),
};
Expand All @@ -3291,12 +3290,13 @@ pub fn gen_std_trampoline_arm64(
)?;
match calling_convention {
CallingConvention::AppleAarch64 => {
caller_stack_offset += match sz {
Size::S8 => 1,
Size::S16 => 2,
Size::S32 => 4,
Size::S64 => 8,
};
caller_stack_offset += 1
<< match sz {
Size::S8 => 0,
Size::S16 => 1,
Size::S32 => 2,
Size::S64 => 3,
};
}
_ => {
caller_stack_offset += 8;
Expand Down
68 changes: 40 additions & 28 deletions lib/compiler-singlepass/src/machine_arm64.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1760,16 +1760,26 @@ impl Machine for MachineARM64 {
.emit_stur(Size::S64, location, GPR::X29, -stack_offset)?;
} else {
let tmp = GPR::X17;
self.assembler
.emit_mov_imm(Location::GPR(tmp), (stack_offset as i64) as u64)?;
self.assembler.emit_sub(
Size::S64,
Location::GPR(GPR::X29),
Location::GPR(tmp),
Location::GPR(tmp),
)?;
self.assembler
.emit_str(Size::S64, location, Location::GPR(tmp))?;
if stack_offset < 0x1_0000 {
self.assembler
.emit_mov_imm(Location::GPR(tmp), (-stack_offset as i64) as u64)?;
self.assembler.emit_str(
Size::S64,
location,
Location::Memory2(GPR::X29, tmp, Multiplier::One, 0),
)?;
} else {
self.assembler
.emit_mov_imm(Location::GPR(tmp), (stack_offset as i64) as u64)?;
self.assembler.emit_sub(
Size::S64,
Location::GPR(GPR::X29),
Location::GPR(tmp),
Location::GPR(tmp),
)?;
self.assembler
.emit_str(Size::S64, location, Location::GPR(tmp))?;
}
}
match location {
Location::GPR(x) => self.emit_unwind_op(UnwindOps::SaveRegister {
Expand Down Expand Up @@ -1809,18 +1819,19 @@ impl Machine for MachineARM64 {
6 => Location::GPR(GPR::X6),
7 => Location::GPR(GPR::X7),
_ => {
let sz = match sz {
Size::S8 => 0,
Size::S16 => 1,
Size::S32 => 2,
Size::S64 => 3,
};
let sz = 1
<< match sz {
Size::S8 => 0,
Size::S16 => 1,
Size::S32 => 2,
Size::S64 => 3,
};
// align first
if sz > 1 && *stack_args & !((1 << sz) - 1) != 0 {
*stack_args = (*stack_args + ((1 << sz) - 1)) & !((1 << sz) - 1);
if sz > 1 && *stack_args & (sz - 1) != 0 {
*stack_args = (*stack_args + (sz - 1)) & !(sz - 1);
}
let loc = Location::Memory(GPR::XzrSp, *stack_args as i32);
*stack_args += 1 << sz;
*stack_args += sz;
loc
}
},
Expand Down Expand Up @@ -1860,18 +1871,19 @@ impl Machine for MachineARM64 {
6 => Location::GPR(GPR::X6),
7 => Location::GPR(GPR::X7),
_ => {
let sz = match sz {
Size::S8 => 0,
Size::S16 => 1,
Size::S32 => 2,
Size::S64 => 3,
};
let sz = 1
<< match sz {
Size::S8 => 0,
Size::S16 => 1,
Size::S32 => 2,
Size::S64 => 3,
};
// align first
if sz > 1 && *stack_args & !((1 << sz) - 1) != 0 {
*stack_args = (*stack_args + ((1 << sz) - 1)) & !((1 << sz) - 1);
if sz > 1 && *stack_args & (sz - 1) != 0 {
*stack_args = (*stack_args + (sz - 1)) & !(sz - 1);
}
let loc = Location::Memory(GPR::X29, 16 * 2 + *stack_args as i32);
*stack_args += 1 << sz;
*stack_args += sz;
loc
}
},
Expand Down
105 changes: 105 additions & 0 deletions tests/compilers/issues.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,3 +317,108 @@ fn test_popcnt(mut config: crate::Config) -> Result<()> {

Ok(())
}

/// Create a large number of local (more than 0x1_0000 bytes, thats 32*16 i64 + 1)
/// to trigger an issue in the arm64 singlepass compiler
/// sequence
/// mov x17, #0x1010
/// sub xsp, xsp, x17
/// will tranform to
/// mov x17, #0x1010
/// sub xzr, xzr, x17
/// and the locals
/// on stack can get corrupted by subsequent calls if they also have locals on stack
#[compiler_test(issues)]
fn large_number_local(mut config: crate::Config) -> Result<()> {
let mut store = config.store();
let wat = r#"
(module
(func (;0;)
(local i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64)
i64.const 0
i64.const 5555
i64.add
local.set 8
i64.const 0
i64.const 5555
i64.add
local.set 9
i64.const 0
i64.const 5555
i64.add
local.set 10
)
(func $large_local (export "large_local") (result i64)
(local
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64
i64
)
(local.set 15 (i64.const 1))
(call 0)
local.get 6
local.get 7
i64.add
local.get 8
i64.add
local.get 9
i64.add
local.get 10
i64.add
local.get 11
i64.add
local.get 12
i64.add
local.get 13
i64.add
local.get 14
i64.add
local.get 15
i64.add
local.get 16
i64.add
)
)
"#;
let mut env = FunctionEnv::new(&mut store, ());
let module = Module::new(&store, wat)?;
let imports: Imports = imports! {};
let instance = Instance::new(&mut store, &module, &imports)?;
let result = instance
.exports
.get_function("large_local")?
.call(&mut store, &[])
.unwrap();
assert_eq!(&Value::I64(1 as i64), result.get(0).unwrap());
Ok(())
}

0 comments on commit 2d17aa9

Please sign in to comment.