Auto merge of rust-lang#122662 - Mark-Simulacrum:optional-drop, r=<try>

Omit non-needs_drop drop_in_place in vtables

This replaces the drop_in_place reference with null in vtables. On librustc_driver.so, this drops about ~17k (11%) dynamic relocations from the output, since many vtables can now be placed in read-only memory, rather than having a relocated pointer included.

This makes a tradeoff by adding a null check at vtable call sites. I'm not sure that's readily avoidable without changing the vtable format (e.g., so that we can use a pc-relative relocation instead of an absolute address, and avoid the dynamic relocation that way). But it seems likely that the check is cheap at runtime.

Accepted MCP: rust-lang/compiler-team#730

Author: bors

compiler/rustc_codegen_ssa/src/meth.rs

@@ -13,7 +13,7 @@ impl<'a, 'tcx> VirtualIndex {
         VirtualIndex(index as u64)
     }
 
-    pub fn get_fn<Bx: BuilderMethods<'a, 'tcx>>(
+    pub fn get_optional_fn<Bx: BuilderMethods<'a, 'tcx>>(
         self,
         bx: &mut Bx,
         llvtable: Bx::Value,
@@ -39,13 +39,24 @@ impl<'a, 'tcx> VirtualIndex {
         } else {
             let gep = bx.inbounds_ptradd(llvtable, bx.const_usize(vtable_byte_offset));
             let ptr = bx.load(llty, gep, ptr_align);
-            bx.nonnull_metadata(ptr);
             // VTable loads are invariant.
             bx.set_invariant_load(ptr);
             ptr
         }
     }
 
+    pub fn get_fn<Bx: BuilderMethods<'a, 'tcx>>(
+        self,
+        bx: &mut Bx,
+        llvtable: Bx::Value,
+        ty: Ty<'tcx>,
+        fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
+    ) -> Bx::Value {
+        let ptr = self.get_optional_fn(bx, llvtable, ty, fn_abi);
+        bx.nonnull_metadata(ptr);
+        ptr
+    }
+
     pub fn get_usize<Bx: BuilderMethods<'a, 'tcx>>(
         self,
         bx: &mut Bx,

compiler/rustc_codegen_ssa/src/mir/block.rs

@@ -498,6 +498,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         &mut self,
         helper: TerminatorCodegenHelper<'tcx>,
         bx: &mut Bx,
+        source_info: &mir::SourceInfo,
         location: mir::Place<'tcx>,
         target: mir::BasicBlock,
         unwind: mir::UnwindAction,
@@ -521,7 +522,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             args1 = [place.val.llval];
             &args1[..]
         };
-        let (drop_fn, fn_abi, drop_instance) =
+        let (maybe_null, drop_fn, fn_abi, drop_instance) =
             match ty.kind() {
                 // FIXME(eddyb) perhaps move some of this logic into
                 // `Instance::resolve_drop_in_place`?
@@ -550,6 +551,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     // Truncate vtable off of args list
                     args = &args[..1];
                     (
+                        true,
                         meth::VirtualIndex::from_index(ty::COMMON_VTABLE_ENTRIES_DROPINPLACE)
                             .get_fn(bx, vtable, ty, fn_abi),
                         fn_abi,
@@ -593,18 +595,37 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     args = &args[..1];
                     debug!("args' = {:?}", args);
                     (
+                        true,
                         meth::VirtualIndex::from_index(ty::COMMON_VTABLE_ENTRIES_DROPINPLACE)
                             .get_fn(bx, meta.immediate(), ty, fn_abi),
                         fn_abi,
                         virtual_drop,
                     )
                 }
                 _ => (
+                    false,
                     bx.get_fn_addr(drop_fn),
                     bx.fn_abi_of_instance(drop_fn, ty::List::empty()),
                     drop_fn,
                 ),
             };
+
+        // We generate a null check for the drop_fn. This saves a bunch of relocations being
+        // generated for no-op drops.
+        if maybe_null {
+            let is_not_null = bx.append_sibling_block("is_not_null");
+            let llty = bx.fn_ptr_backend_type(fn_abi);
+            let null = bx.const_null(llty);
+            let non_null = bx.icmp(
+                base::bin_op_to_icmp_predicate(mir::BinOp::Ne.to_hir_binop(), false),
+                drop_fn,
+                null,
+            );
+            bx.cond_br(non_null, is_not_null, self.llbb(target));
+            bx.switch_to_block(is_not_null);
+            self.set_debug_loc(bx, *source_info);
+        }
+
         helper.do_call(
             self,
             bx,
@@ -615,7 +636,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             unwind,
             &[],
             Some(drop_instance),
-            mergeable_succ,
+            !maybe_null && mergeable_succ,
         )
     }
 
@@ -1344,9 +1365,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 MergingSucc::False
             }
 
-            mir::TerminatorKind::Drop { place, target, unwind, replace: _ } => {
-                self.codegen_drop_terminator(helper, bx, place, target, unwind, mergeable_succ())
-            }
+            mir::TerminatorKind::Drop { place, target, unwind, replace: _ } => self
+                .codegen_drop_terminator(
+                    helper,
+                    bx,
+                    &terminator.source_info,
+                    place,
+                    target,
+                    unwind,
+                    mergeable_succ(),
+                ),
 
             mir::TerminatorKind::Assert { ref cond, expected, ref msg, target, unwind } => self
                 .codegen_assert_terminator(

compiler/rustc_middle/src/ty/vtable.rs

@@ -83,10 +83,14 @@ pub(super) fn vtable_allocation_provider<'tcx>(
         let idx: u64 = u64::try_from(idx).unwrap();
         let scalar = match entry {
             VtblEntry::MetadataDropInPlace => {
-                let instance = ty::Instance::resolve_drop_in_place(tcx, ty);
-                let fn_alloc_id = tcx.reserve_and_set_fn_alloc(instance);
-                let fn_ptr = Pointer::from(fn_alloc_id);
-                Scalar::from_pointer(fn_ptr, &tcx)
+                if ty.needs_drop(tcx, ty::ParamEnv::reveal_all()) {
+                    let instance = ty::Instance::resolve_drop_in_place(tcx, ty);
+                    let fn_alloc_id = tcx.reserve_and_set_fn_alloc(instance);
+                    let fn_ptr = Pointer::from(fn_alloc_id);
+                    Scalar::from_pointer(fn_ptr, &tcx)
+                } else {
+                    Scalar::from_maybe_pointer(Pointer::null(), &tcx)
+                }
             }
             VtblEntry::MetadataSize => Scalar::from_uint(size, ptr_size),
             VtblEntry::MetadataAlign => Scalar::from_uint(align, ptr_size),

tests/codegen/vtable-loads.rs

@@ -0,0 +1,16 @@
+//@ compile-flags: -O
+
+#![crate_type = "lib"]
+
+// CHECK-LABEL: @loop_skips_vtable_load
+#[no_mangle]
+pub fn loop_skips_vtable_load(x: &dyn Fn()) {
+    // CHECK: load ptr, ptr %0{{.*}}, !invariant.load
+    // CHECK-NEXT: tail call void %1
+    // CHECK-NOT: load ptr
+    x();
+    for _ in 0..100 {
+        // CHECK: tail call void %1
+        x();
+    }
+}