From 685cde98a4ef43e3ac43321887e40102655f11bb Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Fri, 23 Mar 2018 13:34:40 -0700 Subject: [PATCH] Mark loads from globals `aligned` and `notrap`. Mark loads from globals generated by cton_wasm or by legalization as `aligned` and `notrap`, since memory for these globals should be allocated by the runtime environment for that purpose. This reduces the number of potentially trapping instructions, which can reduce the amount of metadata required by embedding environments. --- cranelift/docs/langref.rst | 6 ++++++ cranelift/filetests/isa/intel/legalize-memory.cton | 6 +++--- lib/cretonne/meta/base/instructions.py | 3 +++ lib/cretonne/src/ir/globalvar.rs | 3 ++- lib/cretonne/src/ir/heap.rs | 6 ++++-- lib/cretonne/src/legalizer/globalvar.rs | 7 +++++-- lib/cretonne/src/legalizer/heap.rs | 12 ++++++++++-- lib/wasm/src/code_translator.rs | 14 ++++++++++---- lib/wasm/src/environ/dummy.rs | 5 ++++- 9 files changed, 47 insertions(+), 15 deletions(-) diff --git a/cranelift/docs/langref.rst b/cranelift/docs/langref.rst index fb62111d0a..fb716257bd 100644 --- a/cranelift/docs/langref.rst +++ b/cranelift/docs/langref.rst @@ -654,6 +654,9 @@ trap when accessed. address space reserved for the heap, not including the guard pages. :arg GuardBytes: Size of the guard pages in bytes. +When the base is a global variable, it must be :term:`accessible` and naturally +aligned for a pointer value. + The ``reserved_reg`` option is not yet implemented. Dynamic heaps @@ -674,6 +677,9 @@ is resized. The bound of a dynamic heap is stored in a global variable. :arg BoundGV: Global variable containing the current heap bound in bytes. :arg GuardBytes: Size of the guard pages in bytes. +When the base is a global variable, it must be :term:`accessible` and naturally +aligned for a pointer value. + The ``reserved_reg`` option is not yet implemented. Heap examples diff --git a/cranelift/filetests/isa/intel/legalize-memory.cton b/cranelift/filetests/isa/intel/legalize-memory.cton index 5b7246a2e3..1ba979deb1 100644 --- a/cranelift/filetests/isa/intel/legalize-memory.cton +++ b/cranelift/filetests/isa/intel/legalize-memory.cton @@ -23,7 +23,7 @@ function %deref(i64 vmctx) -> i64 { ebb1(v1: i64): v2 = global_addr.i64 gv2 ; check: $(a1=$V) = iadd_imm v1, -16 - ; check: $(p1=$V) = load.i64 $a1 + ; check: $(p1=$V) = load.i64 notrap aligned $a1 ; check: v2 = iadd_imm $p1, 32 return v2 ; check: return v2 @@ -55,7 +55,7 @@ ebb0(v0: i32, v999: i64): ; Checks here are assuming that no pipehole opts fold the load offsets. ; nextln: $(xoff=$V) = uextend.i64 v0 ; nextln: $(haddr=$V) = iadd_imm v999, 64 - ; nextln: $(hbase=$V) = load.i64 $haddr + ; nextln: $(hbase=$V) = load.i64 notrap aligned $haddr ; nextln: v1 = iadd $hbase, $xoff v2 = load.f32 v1+16 ; nextln: v2 = load.f32 v1+16 @@ -103,7 +103,7 @@ ebb0(v0: i32, v999: i64): ; Checks here are assuming that no pipehole opts fold the load offsets. ; nextln: $(xoff=$V) = uextend.i64 v0 ; nextln: $(haddr=$V) = iadd_imm.i64 v999, 64 - ; nextln: $(hbase=$V) = load.i64 $haddr + ; nextln: $(hbase=$V) = load.i64 notrap aligned $haddr ; nextln: v1 = iadd $hbase, $xoff v2 = load.f32 v1+0x7fff_ffff ; nextln: v2 = load.f32 v1+0x7fff_ffff diff --git a/lib/cretonne/meta/base/instructions.py b/lib/cretonne/meta/base/instructions.py index 8ba1aa65fa..c54f3ef570 100644 --- a/lib/cretonne/meta/base/instructions.py +++ b/lib/cretonne/meta/base/instructions.py @@ -588,6 +588,9 @@ stack_check = Instruction( Read the stack limit from ``GV`` and compare it to the stack pointer. If the stack pointer has reached or exceeded the limit, generate a trap with a ``stk_ovf`` code. + + The global variable must be accessible and naturally aligned for a + pointer-sized value. """, ins=GV, can_trap=True) diff --git a/lib/cretonne/src/ir/globalvar.rs b/lib/cretonne/src/ir/globalvar.rs index 78e1c6cb98..c8fc3b8ce1 100644 --- a/lib/cretonne/src/ir/globalvar.rs +++ b/lib/cretonne/src/ir/globalvar.rs @@ -17,7 +17,8 @@ pub enum GlobalVarData { /// Variable is part of a struct pointed to by another global variable. /// /// The `base` global variable is assumed to contain a pointer to a struct. This global - /// variable lives at an offset into the struct. + /// variable lives at an offset into the struct. The memory must be accessible, and + /// naturally aligned to hold a pointer value. Deref { /// The base pointer global variable. base: GlobalVar, diff --git a/lib/cretonne/src/ir/heap.rs b/lib/cretonne/src/ir/heap.rs index e2a5366a1a..b41565b25b 100644 --- a/lib/cretonne/src/ir/heap.rs +++ b/lib/cretonne/src/ir/heap.rs @@ -29,7 +29,8 @@ pub enum HeapBase { /// This feature is not yet implemented. ReservedReg, - /// The heap base is in a global variable. + /// The heap base is in a global variable. The variable must be accessible and naturally + /// aligned for a pointer. GlobalVar(GlobalVar), } @@ -38,7 +39,8 @@ pub enum HeapBase { pub enum HeapStyle { /// A dynamic heap can be relocated to a different base address when it is grown. Dynamic { - /// Global variable holding the current bound of the heap in bytes. + /// Global variable holding the current bound of the heap in bytes. It is + /// required to be accessible and naturally aligned for a pointer-sized integer. bound_gv: GlobalVar, }, diff --git a/lib/cretonne/src/legalizer/globalvar.rs b/lib/cretonne/src/legalizer/globalvar.rs index 5f6427822b..901b0da416 100644 --- a/lib/cretonne/src/legalizer/globalvar.rs +++ b/lib/cretonne/src/legalizer/globalvar.rs @@ -52,8 +52,11 @@ fn deref_addr(inst: ir::Inst, func: &mut ir::Function, base: ir::GlobalVar, offs pos.use_srcloc(inst); let base_addr = pos.ins().global_addr(ptr_ty, base); - // TODO: We could probably set both `notrap` and `aligned` on this load instruction. - let base_ptr = pos.ins().load(ptr_ty, ir::MemFlags::new(), base_addr, 0); + let mut mflags = ir::MemFlags::new(); + // Deref globals are required to be accessible and aligned. + mflags.set_notrap(); + mflags.set_aligned(); + let base_ptr = pos.ins().load(ptr_ty, mflags, base_addr, 0); pos.func.dfg.replace(inst).iadd_imm(base_ptr, offset); } diff --git a/lib/cretonne/src/legalizer/heap.rs b/lib/cretonne/src/legalizer/heap.rs index 37ce226351..0d6e7d385b 100644 --- a/lib/cretonne/src/legalizer/heap.rs +++ b/lib/cretonne/src/legalizer/heap.rs @@ -58,7 +58,11 @@ fn dynamic_addr( // Start with the bounds check. Trap if `offset + size > bound`. let bound_addr = pos.ins().global_addr(addr_ty, bound_gv); - let bound = pos.ins().load(offset_ty, MemFlags::new(), bound_addr, 0); + let mut mflags = MemFlags::new(); + // The bound variable is requied to be accessible and aligned. + mflags.set_notrap(); + mflags.set_aligned(); + let bound = pos.ins().load(offset_ty, mflags, bound_addr, 0); let oob; if size == 1 { @@ -175,7 +179,11 @@ fn offset_addr( ir::HeapBase::ReservedReg => unimplemented!(), ir::HeapBase::GlobalVar(base_gv) => { let base_addr = pos.ins().global_addr(addr_ty, base_gv); - let base = pos.ins().load(addr_ty, MemFlags::new(), base_addr, 0); + let mut mflags = MemFlags::new(); + // The base address variable is requied to be accessible and aligned. + mflags.set_notrap(); + mflags.set_aligned(); + let base = pos.ins().load(addr_ty, mflags, base_addr, 0); pos.func.dfg.replace(inst).iadd(base, offset); } } diff --git a/lib/wasm/src/code_translator.rs b/lib/wasm/src/code_translator.rs index 42d232e6f5..69111dc8e8 100644 --- a/lib/wasm/src/code_translator.rs +++ b/lib/wasm/src/code_translator.rs @@ -75,8 +75,9 @@ pub fn translate_operator( GlobalValue::Const(val) => val, GlobalValue::Memory { gv, ty } => { let addr = builder.ins().global_addr(environ.native_pointer(), gv); - // TODO: It is likely safe to set `aligned notrap` flags on a global load. - let flags = ir::MemFlags::new(); + let mut flags = ir::MemFlags::new(); + flags.set_notrap(); + flags.set_aligned(); builder.ins().load(ty, flags, addr, 0) } }; @@ -87,8 +88,9 @@ pub fn translate_operator( GlobalValue::Const(_) => panic!("global #{} is a constant", global_index), GlobalValue::Memory { gv, .. } => { let addr = builder.ins().global_addr(environ.native_pointer(), gv); - // TODO: It is likely safe to set `aligned notrap` flags on a global store. - let flags = ir::MemFlags::new(); + let mut flags = ir::MemFlags::new(); + flags.set_notrap(); + flags.set_aligned(); let val = state.pop1(); builder.ins().store(flags, val, addr, 0); } @@ -992,6 +994,9 @@ fn translate_load( // We don't yet support multiple linear memories. let heap = state.get_heap(builder.func, 0, environ); let (base, offset) = get_heap_addr(heap, addr32, offset, environ.native_pointer(), builder); + // Note that we don't set `is_aligned` here, even if the load instruction's + // alignment immediate says it's aligned, because WebAssembly's immediate + // field is just a hint, while Cretonne's aligned flag needs a guarantee. let flags = MemFlags::new(); let (load, dfg) = builder.ins().Load( opcode, @@ -1017,6 +1022,7 @@ fn translate_store( // We don't yet support multiple linear memories. let heap = state.get_heap(builder.func, 0, environ); let (base, offset) = get_heap_addr(heap, addr32, offset, environ.native_pointer(), builder); + // See the comments in `translate_load` about the flags. let flags = MemFlags::new(); builder.ins().Store( opcode, diff --git a/lib/wasm/src/environ/dummy.rs b/lib/wasm/src/environ/dummy.rs index c99afd32d9..bb21b7536a 100644 --- a/lib/wasm/src/environ/dummy.rs +++ b/lib/wasm/src/environ/dummy.rs @@ -209,7 +209,10 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ let ext = pos.ins().uextend(I64, callee); pos.ins().imul_imm(ext, 4) }; - let func_ptr = pos.ins().load(ptr, ir::MemFlags::new(), callee_offset, 0); + let mut mflags = ir::MemFlags::new(); + mflags.set_notrap(); + mflags.set_aligned(); + let func_ptr = pos.ins().load(ptr, mflags, callee_offset, 0); // Build a value list for the indirect call instruction containing the callee, call_args, // and the vmctx parameter.