From b638f11b4230ad2e098a75474c36a10de1653d0d Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Fri, 11 May 2018 11:26:51 +0200 Subject: [PATCH 1/2] Introduce OperandValue::volatile_store and use it in the intrinsics Fixes #50371. --- src/librustc_trans/abi.rs | 3 ++- src/librustc_trans/base.rs | 10 ++++++---- src/librustc_trans/builder.rs | 3 ++- src/librustc_trans/intrinsic.rs | 21 +++------------------ src/librustc_trans/mir/block.rs | 2 +- src/librustc_trans/mir/operand.rs | 24 +++++++++++++++++++++--- 6 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/librustc_trans/abi.rs b/src/librustc_trans/abi.rs index 1838dae049ad7..c80d989e3cb18 100644 --- a/src/librustc_trans/abi.rs +++ b/src/librustc_trans/abi.rs @@ -220,7 +220,8 @@ impl<'a, 'tcx> ArgTypeExt<'a, 'tcx> for ArgType<'tcx, Ty<'tcx>> { bx.pointercast(dst.llval, Type::i8p(cx)), bx.pointercast(llscratch, Type::i8p(cx)), C_usize(cx, self.layout.size.bytes()), - self.layout.align.min(scratch_align)); + self.layout.align.min(scratch_align), + false); bx.lifetime_end(llscratch, scratch_size); } diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index 0dd1adbff86e0..65dce5f3ffbce 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -320,7 +320,7 @@ pub fn coerce_unsized_into<'a, 'tcx>(bx: &Builder<'a, 'tcx>, if src_f.layout.ty == dst_f.layout.ty { memcpy_ty(bx, dst_f.llval, src_f.llval, src_f.layout, - src_f.align.min(dst_f.align)); + src_f.align.min(dst_f.align), false); } else { coerce_unsized_into(bx, src_f, dst_f); } @@ -408,7 +408,8 @@ pub fn call_memcpy(bx: &Builder, dst: ValueRef, src: ValueRef, n_bytes: ValueRef, - align: Align) { + align: Align, + volatile: bool) { let cx = bx.cx; let ptr_width = &cx.sess().target.target.target_pointer_width; let key = format!("llvm.memcpy.p0i8.p0i8.i{}", ptr_width); @@ -417,7 +418,7 @@ pub fn call_memcpy(bx: &Builder, let dst_ptr = bx.pointercast(dst, Type::i8p(cx)); let size = bx.intcast(n_bytes, cx.isize_ty, false); let align = C_i32(cx, align.abi() as i32); - let volatile = C_bool(cx, false); + let volatile = C_bool(cx, volatile); bx.call(memcpy, &[dst_ptr, src_ptr, size, align, volatile], None); } @@ -427,13 +428,14 @@ pub fn memcpy_ty<'a, 'tcx>( src: ValueRef, layout: TyLayout<'tcx>, align: Align, + volatile: bool, ) { let size = layout.size.bytes(); if size == 0 { return; } - call_memcpy(bx, dst, src, C_usize(bx.cx, size), align); + call_memcpy(bx, dst, src, C_usize(bx.cx, size), align, volatile); } pub fn call_memset<'a, 'tcx>(bx: &Builder<'a, 'tcx>, diff --git a/src/librustc_trans/builder.rs b/src/librustc_trans/builder.rs index db803ca8209d9..49bcf9b88a065 100644 --- a/src/librustc_trans/builder.rs +++ b/src/librustc_trans/builder.rs @@ -590,13 +590,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } - pub fn volatile_store(&self, val: ValueRef, ptr: ValueRef) -> ValueRef { + pub fn volatile_store(&self, val: ValueRef, ptr: ValueRef, align: Align) -> ValueRef { debug!("Store {:?} -> {:?}", Value(val), Value(ptr)); assert!(!self.llbuilder.is_null()); self.count_insn("store.volatile"); let ptr = self.check_store(val, ptr); unsafe { let insn = llvm::LLVMBuildStore(self.llbuilder, val, ptr); + llvm::LLVMSetAlignment(insn, align.abi() as c_uint); llvm::LLVMSetVolatile(insn, llvm::True); insn } diff --git a/src/librustc_trans/intrinsic.rs b/src/librustc_trans/intrinsic.rs index 49a207a2d8ab5..65e211ae740bc 100644 --- a/src/librustc_trans/intrinsic.rs +++ b/src/librustc_trans/intrinsic.rs @@ -247,26 +247,11 @@ pub fn trans_intrinsic_call<'a, 'tcx>(bx: &Builder<'a, 'tcx>, to_immediate(bx, load, cx.layout_of(tp_ty)) }, "volatile_store" => { - let tp_ty = substs.type_at(0); let dst = args[0].deref(bx.cx); - if let OperandValue::Pair(a, b) = args[1].val { - bx.volatile_store(a, dst.project_field(bx, 0).llval); - bx.volatile_store(b, dst.project_field(bx, 1).llval); - } else { - let val = if let OperandValue::Ref(ptr, align) = args[1].val { - bx.load(ptr, align) - } else { - if dst.layout.is_zst() { - return; - } - from_immediate(bx, args[1].immediate()) - }; - let ptr = bx.pointercast(dst.llval, val_ty(val).ptr_to()); - let store = bx.volatile_store(val, ptr); - unsafe { - llvm::LLVMSetAlignment(store, cx.align_of(tp_ty).abi() as u32); - } + if dst.layout.is_zst() { + return; } + args[1].val.volatile_store(bx, dst); return; }, "prefetch_read_data" | "prefetch_write_data" | diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index b666c2b211525..df8807c318b17 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -626,7 +626,7 @@ impl<'a, 'tcx> FunctionCx<'a, 'tcx> { // have scary latent bugs around. let scratch = PlaceRef::alloca(bx, arg.layout, "arg"); - base::memcpy_ty(bx, scratch.llval, llval, op.layout, align); + base::memcpy_ty(bx, scratch.llval, llval, op.layout, align, false); (scratch.llval, scratch.align, true) } else { (llval, align, true) diff --git a/src/librustc_trans/mir/operand.rs b/src/librustc_trans/mir/operand.rs index 432ac44e0a566..f69661782915f 100644 --- a/src/librustc_trans/mir/operand.rs +++ b/src/librustc_trans/mir/operand.rs @@ -275,6 +275,14 @@ impl<'a, 'tcx> OperandRef<'tcx> { impl<'a, 'tcx> OperandValue { pub fn store(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>) { + self.store_maybe_volatile(bx, dest, false); + } + + pub fn volatile_store(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>) { + self.store_maybe_volatile(bx, dest, true); + } + + fn store_maybe_volatile(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>, volatile: bool) { debug!("OperandRef::store: operand={:?}, dest={:?}", self, dest); // Avoid generating stores of zero-sized values, because the only way to have a zero-sized // value is through `undef`, and store itself is useless. @@ -284,9 +292,14 @@ impl<'a, 'tcx> OperandValue { match self { OperandValue::Ref(r, source_align) => base::memcpy_ty(bx, dest.llval, r, dest.layout, - source_align.min(dest.align)), + source_align.min(dest.align), volatile), OperandValue::Immediate(s) => { - bx.store(base::from_immediate(bx, s), dest.llval, dest.align); + let val = base::from_immediate(bx, s); + if !volatile { + bx.store(val, dest.llval, dest.align); + } else { + bx.volatile_store(val, dest.llval, dest.align); + } } OperandValue::Pair(a, b) => { for (i, &x) in [a, b].iter().enumerate() { @@ -295,7 +308,12 @@ impl<'a, 'tcx> OperandValue { if common::val_ty(x) == Type::i1(bx.cx) { llptr = bx.pointercast(llptr, Type::i8p(bx.cx)); } - bx.store(base::from_immediate(bx, x), llptr, dest.align); + let val = base::from_immediate(bx, x); + if !volatile { + bx.store(val, llptr, dest.align); + } else { + bx.volatile_store(val, llptr, dest.align); + } } } } From 3ebe8679d2eed3caf30d8c6164405916de38ef94 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Fri, 11 May 2018 12:26:32 +0200 Subject: [PATCH 2/2] Introduce OperandValue::nontemporal_store and use it in the intrinsics We use a new MemFlags bitflags type to merge some store code paths. --- src/Cargo.lock | 1 + src/librustc_trans/Cargo.toml | 1 + src/librustc_trans/abi.rs | 4 +- src/librustc_trans/base.rs | 19 ++++++--- src/librustc_trans/builder.rs | 67 ++++++++++++++----------------- src/librustc_trans/intrinsic.rs | 17 +------- src/librustc_trans/lib.rs | 1 + src/librustc_trans/mir/block.rs | 4 +- src/librustc_trans/mir/operand.rs | 29 ++++++------- 9 files changed, 65 insertions(+), 78 deletions(-) diff --git a/src/Cargo.lock b/src/Cargo.lock index 4cb82f94251ff..70a230cb7f7ac 100644 --- a/src/Cargo.lock +++ b/src/Cargo.lock @@ -2178,6 +2178,7 @@ dependencies = [ name = "rustc_trans" version = "0.0.0" dependencies = [ + "bitflags 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "cc 1.0.15 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.5.8 (registry+https://github.com/rust-lang/crates.io-index)", "flate2 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/src/librustc_trans/Cargo.toml b/src/librustc_trans/Cargo.toml index a4dd02e97b233..64d3a4f4d53be 100644 --- a/src/librustc_trans/Cargo.toml +++ b/src/librustc_trans/Cargo.toml @@ -10,6 +10,7 @@ crate-type = ["dylib"] test = false [dependencies] +bitflags = "1.0.1" cc = "1.0.1" flate2 = "1.0" jobserver = "0.1.5" diff --git a/src/librustc_trans/abi.rs b/src/librustc_trans/abi.rs index c80d989e3cb18..25c598c532c48 100644 --- a/src/librustc_trans/abi.rs +++ b/src/librustc_trans/abi.rs @@ -10,7 +10,7 @@ use llvm::{self, ValueRef, AttributePlace}; use base; -use builder::Builder; +use builder::{Builder, MemFlags}; use common::{ty_fn_sig, C_usize}; use context::CodegenCx; use mir::place::PlaceRef; @@ -221,7 +221,7 @@ impl<'a, 'tcx> ArgTypeExt<'a, 'tcx> for ArgType<'tcx, Ty<'tcx>> { bx.pointercast(llscratch, Type::i8p(cx)), C_usize(cx, self.layout.size.bytes()), self.layout.align.min(scratch_align), - false); + MemFlags::empty()); bx.lifetime_end(llscratch, scratch_size); } diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index 65dce5f3ffbce..feca36fa6c243 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -53,7 +53,7 @@ use rustc_incremental; use allocator; use mir::place::PlaceRef; use attributes; -use builder::Builder; +use builder::{Builder, MemFlags}; use callee; use common::{C_bool, C_bytes_in_context, C_i32, C_usize}; use rustc_mir::monomorphize::collector::{self, MonoItemCollectionMode}; @@ -320,7 +320,7 @@ pub fn coerce_unsized_into<'a, 'tcx>(bx: &Builder<'a, 'tcx>, if src_f.layout.ty == dst_f.layout.ty { memcpy_ty(bx, dst_f.llval, src_f.llval, src_f.layout, - src_f.align.min(dst_f.align), false); + src_f.align.min(dst_f.align), MemFlags::empty()); } else { coerce_unsized_into(bx, src_f, dst_f); } @@ -409,7 +409,14 @@ pub fn call_memcpy(bx: &Builder, src: ValueRef, n_bytes: ValueRef, align: Align, - volatile: bool) { + flags: MemFlags) { + if flags.contains(MemFlags::NONTEMPORAL) { + // HACK(nox): This is inefficient but there is no nontemporal memcpy. + let val = bx.load(src, align); + let ptr = bx.pointercast(dst, val_ty(val).ptr_to()); + bx.store_with_flags(val, ptr, align, flags); + return; + } let cx = bx.cx; let ptr_width = &cx.sess().target.target.target_pointer_width; let key = format!("llvm.memcpy.p0i8.p0i8.i{}", ptr_width); @@ -418,7 +425,7 @@ pub fn call_memcpy(bx: &Builder, let dst_ptr = bx.pointercast(dst, Type::i8p(cx)); let size = bx.intcast(n_bytes, cx.isize_ty, false); let align = C_i32(cx, align.abi() as i32); - let volatile = C_bool(cx, volatile); + let volatile = C_bool(cx, flags.contains(MemFlags::VOLATILE)); bx.call(memcpy, &[dst_ptr, src_ptr, size, align, volatile], None); } @@ -428,14 +435,14 @@ pub fn memcpy_ty<'a, 'tcx>( src: ValueRef, layout: TyLayout<'tcx>, align: Align, - volatile: bool, + flags: MemFlags, ) { let size = layout.size.bytes(); if size == 0 { return; } - call_memcpy(bx, dst, src, C_usize(bx.cx, size), align, volatile); + call_memcpy(bx, dst, src, C_usize(bx.cx, size), align, flags); } pub fn call_memset<'a, 'tcx>(bx: &Builder<'a, 'tcx>, diff --git a/src/librustc_trans/builder.rs b/src/librustc_trans/builder.rs index 49bcf9b88a065..4153c61e5269a 100644 --- a/src/librustc_trans/builder.rs +++ b/src/librustc_trans/builder.rs @@ -50,6 +50,13 @@ fn noname() -> *const c_char { &CNULL } +bitflags! { + pub struct MemFlags: u8 { + const VOLATILE = 1 << 0; + const NONTEMPORAL = 1 << 1; + } +} + impl<'a, 'tcx> Builder<'a, 'tcx> { pub fn new_block<'b>(cx: &'a CodegenCx<'a, 'tcx>, llfn: ValueRef, name: &'b str) -> Self { let bx = Builder::with_cx(cx); @@ -579,30 +586,39 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } pub fn store(&self, val: ValueRef, ptr: ValueRef, align: Align) -> ValueRef { - debug!("Store {:?} -> {:?}", Value(val), Value(ptr)); + self.store_with_flags(val, ptr, align, MemFlags::empty()) + } + + pub fn store_with_flags( + &self, + val: ValueRef, + ptr: ValueRef, + align: Align, + flags: MemFlags, + ) -> ValueRef { + debug!("Store {:?} -> {:?} ({:?})", Value(val), Value(ptr), flags); assert!(!self.llbuilder.is_null()); self.count_insn("store"); let ptr = self.check_store(val, ptr); unsafe { let store = llvm::LLVMBuildStore(self.llbuilder, val, ptr); llvm::LLVMSetAlignment(store, align.abi() as c_uint); + if flags.contains(MemFlags::VOLATILE) { + llvm::LLVMSetVolatile(store, llvm::True); + } + if flags.contains(MemFlags::NONTEMPORAL) { + // According to LLVM [1] building a nontemporal store must + // *always* point to a metadata value of the integer 1. + // + // [1]: http://llvm.org/docs/LangRef.html#store-instruction + let one = C_i32(self.cx, 1); + let node = llvm::LLVMMDNodeInContext(self.cx.llcx, &one, 1); + llvm::LLVMSetMetadata(store, llvm::MD_nontemporal as c_uint, node); + } store } } - pub fn volatile_store(&self, val: ValueRef, ptr: ValueRef, align: Align) -> ValueRef { - debug!("Store {:?} -> {:?}", Value(val), Value(ptr)); - assert!(!self.llbuilder.is_null()); - self.count_insn("store.volatile"); - let ptr = self.check_store(val, ptr); - unsafe { - let insn = llvm::LLVMBuildStore(self.llbuilder, val, ptr); - llvm::LLVMSetAlignment(insn, align.abi() as c_uint); - llvm::LLVMSetVolatile(insn, llvm::True); - insn - } - } - pub fn atomic_store(&self, val: ValueRef, ptr: ValueRef, order: AtomicOrdering, align: Align) { debug!("Store {:?} -> {:?}", Value(val), Value(ptr)); @@ -616,29 +632,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } - pub fn nontemporal_store(&self, val: ValueRef, ptr: ValueRef) -> ValueRef { - debug!("Store {:?} -> {:?}", Value(val), Value(ptr)); - assert!(!self.llbuilder.is_null()); - self.count_insn("store.nontemporal"); - let ptr = self.check_store(val, ptr); - unsafe { - let insn = llvm::LLVMBuildStore(self.llbuilder, val, ptr); - - // According to LLVM [1] building a nontemporal store must *always* - // point to a metadata value of the integer 1. Who knew? - // - // [1]: http://llvm.org/docs/LangRef.html#store-instruction - let one = C_i32(self.cx, 1); - let node = llvm::LLVMMDNodeInContext(self.cx.llcx, - &one, - 1); - llvm::LLVMSetMetadata(insn, - llvm::MD_nontemporal as c_uint, - node); - insn - } - } - pub fn gep(&self, ptr: ValueRef, indices: &[ValueRef]) -> ValueRef { self.count_insn("gep"); unsafe { diff --git a/src/librustc_trans/intrinsic.rs b/src/librustc_trans/intrinsic.rs index 65e211ae740bc..86aa48b6a9e48 100644 --- a/src/librustc_trans/intrinsic.rs +++ b/src/librustc_trans/intrinsic.rs @@ -248,9 +248,6 @@ pub fn trans_intrinsic_call<'a, 'tcx>(bx: &Builder<'a, 'tcx>, }, "volatile_store" => { let dst = args[0].deref(bx.cx); - if dst.layout.is_zst() { - return; - } args[1].val.volatile_store(bx, dst); return; }, @@ -536,19 +533,9 @@ pub fn trans_intrinsic_call<'a, 'tcx>(bx: &Builder<'a, 'tcx>, } "nontemporal_store" => { - let tp_ty = substs.type_at(0); let dst = args[0].deref(bx.cx); - let val = if let OperandValue::Ref(ptr, align) = args[1].val { - bx.load(ptr, align) - } else { - from_immediate(bx, args[1].immediate()) - }; - let ptr = bx.pointercast(dst.llval, val_ty(val).ptr_to()); - let store = bx.nontemporal_store(val, ptr); - unsafe { - llvm::LLVMSetAlignment(store, cx.align_of(tp_ty).abi() as u32); - } - return + args[1].val.nontemporal_store(bx, dst); + return; } _ => { diff --git a/src/librustc_trans/lib.rs b/src/librustc_trans/lib.rs index 30780b8c96563..6db95657ce058 100644 --- a/src/librustc_trans/lib.rs +++ b/src/librustc_trans/lib.rs @@ -34,6 +34,7 @@ use rustc::dep_graph::WorkProduct; use syntax_pos::symbol::Symbol; +#[macro_use] extern crate bitflags; extern crate flate2; extern crate libc; #[macro_use] extern crate rustc; diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index df8807c318b17..e4989da36c026 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -17,7 +17,7 @@ use rustc::mir::interpret::EvalErrorKind; use abi::{Abi, ArgType, ArgTypeExt, FnType, FnTypeExt, LlvmType, PassMode}; use base; use callee; -use builder::Builder; +use builder::{Builder, MemFlags}; use common::{self, C_bool, C_str_slice, C_struct, C_u32, C_uint_big, C_undef}; use consts; use meth; @@ -626,7 +626,7 @@ impl<'a, 'tcx> FunctionCx<'a, 'tcx> { // have scary latent bugs around. let scratch = PlaceRef::alloca(bx, arg.layout, "arg"); - base::memcpy_ty(bx, scratch.llval, llval, op.layout, align, false); + base::memcpy_ty(bx, scratch.llval, llval, op.layout, align, MemFlags::empty()); (scratch.llval, scratch.align, true) } else { (llval, align, true) diff --git a/src/librustc_trans/mir/operand.rs b/src/librustc_trans/mir/operand.rs index f69661782915f..3e4a73c2e905f 100644 --- a/src/librustc_trans/mir/operand.rs +++ b/src/librustc_trans/mir/operand.rs @@ -18,7 +18,7 @@ use rustc_data_structures::indexed_vec::Idx; use base; use common::{self, CodegenCx, C_null, C_undef, C_usize}; -use builder::Builder; +use builder::{Builder, MemFlags}; use value::Value; use type_of::LayoutLlvmExt; use type_::Type; @@ -275,14 +275,18 @@ impl<'a, 'tcx> OperandRef<'tcx> { impl<'a, 'tcx> OperandValue { pub fn store(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>) { - self.store_maybe_volatile(bx, dest, false); + self.store_with_flags(bx, dest, MemFlags::empty()); } pub fn volatile_store(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>) { - self.store_maybe_volatile(bx, dest, true); + self.store_with_flags(bx, dest, MemFlags::VOLATILE); } - fn store_maybe_volatile(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>, volatile: bool) { + pub fn nontemporal_store(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>) { + self.store_with_flags(bx, dest, MemFlags::NONTEMPORAL); + } + + fn store_with_flags(self, bx: &Builder<'a, 'tcx>, dest: PlaceRef<'tcx>, flags: MemFlags) { debug!("OperandRef::store: operand={:?}, dest={:?}", self, dest); // Avoid generating stores of zero-sized values, because the only way to have a zero-sized // value is through `undef`, and store itself is useless. @@ -290,16 +294,13 @@ impl<'a, 'tcx> OperandValue { return; } match self { - OperandValue::Ref(r, source_align) => + OperandValue::Ref(r, source_align) => { base::memcpy_ty(bx, dest.llval, r, dest.layout, - source_align.min(dest.align), volatile), + source_align.min(dest.align), flags) + } OperandValue::Immediate(s) => { let val = base::from_immediate(bx, s); - if !volatile { - bx.store(val, dest.llval, dest.align); - } else { - bx.volatile_store(val, dest.llval, dest.align); - } + bx.store_with_flags(val, dest.llval, dest.align, flags); } OperandValue::Pair(a, b) => { for (i, &x) in [a, b].iter().enumerate() { @@ -309,11 +310,7 @@ impl<'a, 'tcx> OperandValue { llptr = bx.pointercast(llptr, Type::i8p(bx.cx)); } let val = base::from_immediate(bx, x); - if !volatile { - bx.store(val, llptr, dest.align); - } else { - bx.volatile_store(val, llptr, dest.align); - } + bx.store_with_flags(val, llptr, dest.align, flags); } } }