Auto merge of rust-lang#68961 - eddyb:dbg-stack-dunk, r=nagisa

rustc_codegen_ssa: only "spill" SSA-like values to the stack for debuginfo.

This is an implementation of the idea described in rust-lang#68817 (comment).

In short, instead of debuginfo forcing otherwise-SSA-like MIR locals into `alloca`s, and requiring a `load` for each use (or two, for scalar pairs), the `alloca` is now *only* used for attaching debuginfo with `llvm.dbg.declare`: the `OperandRef` is stored to the `alloca`, but *never loaded* from it.

Outside of `debug_introduce_local`, nothing cares about the debuginfo-only `alloca`, and instead works with `OperandRef` the same as MIR locals without debuginfo before this PR.

This should have some of the benefits of `llvm.dbg.value`, while working today.

cc @nagisa @nikomatsakis

Author: bors

src/librustc_codegen_ssa/mir/analyze.rs

@@ -8,7 +8,6 @@ use rustc::mir::visit::{
     MutatingUseContext, NonMutatingUseContext, NonUseContext, PlaceContext, Visitor,
 };
 use rustc::mir::{self, Location, TerminatorKind};
-use rustc::session::config::DebugInfo;
 use rustc::ty;
 use rustc::ty::layout::{HasTyCtxt, LayoutOf};
 use rustc_data_structures::graph::dominators::Dominators;
@@ -24,15 +23,6 @@ pub fn non_ssa_locals<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
     analyzer.visit_body(mir);
 
     for (local, decl) in mir.local_decls.iter_enumerated() {
-        // FIXME(eddyb): We should figure out how to use llvm.dbg.value instead
-        // of putting everything in allocas just so we can use llvm.dbg.declare.
-        if fx.cx.sess().opts.debuginfo == DebugInfo::Full {
-            if fx.mir.local_kind(local) == mir::LocalKind::Arg {
-                analyzer.not_ssa(local);
-                continue;
-            }
-        }
-
         let ty = fx.monomorphize(&decl.ty);
         debug!("local {:?} has type `{}`", local, ty);
         let layout = fx.cx.spanned_layout_of(ty, decl.source_info.span);
@@ -281,15 +271,6 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx>
                 self.assign(local, location);
             }
 
-            PlaceContext::NonUse(NonUseContext::VarDebugInfo) => {
-                // We need to keep locals in `alloca`s for debuginfo.
-                // FIXME(eddyb): We should figure out how to use `llvm.dbg.value` instead
-                // of putting everything in allocas just so we can use `llvm.dbg.declare`.
-                if self.fx.cx.sess().opts.debuginfo == DebugInfo::Full {
-                    self.not_ssa(local);
-                }
-            }
-
             PlaceContext::NonUse(_) | PlaceContext::MutatingUse(MutatingUseContext::Retag) => {}
 
             PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy)

src/librustc_codegen_ssa/mir/block.rs

@@ -1145,6 +1145,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     let op = bx.load_operand(place);
                     place.storage_dead(bx);
                     self.locals[index] = LocalRef::Operand(Some(op));
+                    self.debug_introduce_local(bx, index);
                 }
                 LocalRef::Operand(Some(op)) => {
                     assert!(op.layout.is_zst(), "assigning to initialized SSAtemp");
@@ -1186,6 +1187,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 let op = bx.load_operand(tmp);
                 tmp.storage_dead(bx);
                 self.locals[index] = LocalRef::Operand(Some(op));
+                self.debug_introduce_local(bx, index);
             }
             DirectOperand(index) => {
                 // If there is a cast, we have to store and reload.
@@ -1200,6 +1202,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     OperandRef::from_immediate_or_packed_pair(bx, llval, ret_abi.layout)
                 };
                 self.locals[index] = LocalRef::Operand(Some(op));
+                self.debug_introduce_local(bx, index);
             }
         }
     }

src/librustc_codegen_ssa/mir/debuginfo.rs

@@ -9,7 +9,8 @@ use rustc_index::vec::IndexVec;
 use rustc_span::symbol::{kw, Symbol};
 use rustc_span::{BytePos, Span};
 
-use super::OperandValue;
+use super::operand::OperandValue;
+use super::place::PlaceRef;
 use super::{FunctionCx, LocalRef};
 
 pub struct FunctionDebugContext<D> {
@@ -111,8 +112,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
 
     /// Apply debuginfo and/or name, after creating the `alloca` for a local,
     /// or initializing the local with an operand (whichever applies).
-    // FIXME(eddyb) use `llvm.dbg.value` (which would work for operands),
-    // not just `llvm.dbg.declare` (which requires `alloca`).
     pub fn debug_introduce_local(&self, bx: &mut Bx, local: mir::Local) {
         let full_debug_info = bx.sess().opts.debuginfo == DebugInfo::Full;
 
@@ -180,38 +179,60 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
 
         let local_ref = &self.locals[local];
 
-        if !bx.sess().fewer_names() {
-            let name = match whole_local_var.or(fallback_var) {
+        let name = if bx.sess().fewer_names() {
+            None
+        } else {
+            Some(match whole_local_var.or(fallback_var) {
                 Some(var) if var.name != kw::Invalid => var.name.to_string(),
                 _ => format!("{:?}", local),
-            };
+            })
+        };
+
+        if let Some(name) = &name {
             match local_ref {
                 LocalRef::Place(place) | LocalRef::UnsizedPlace(place) => {
-                    bx.set_var_name(place.llval, &name);
+                    bx.set_var_name(place.llval, name);
                 }
                 LocalRef::Operand(Some(operand)) => match operand.val {
                     OperandValue::Ref(x, ..) | OperandValue::Immediate(x) => {
-                        bx.set_var_name(x, &name);
+                        bx.set_var_name(x, name);
                     }
                     OperandValue::Pair(a, b) => {
                         // FIXME(eddyb) these are scalar components,
                         // maybe extract the high-level fields?
                         bx.set_var_name(a, &(name.clone() + ".0"));
-                        bx.set_var_name(b, &(name + ".1"));
+                        bx.set_var_name(b, &(name.clone() + ".1"));
                     }
                 },
                 LocalRef::Operand(None) => {}
             }
         }
 
-        if !full_debug_info {
+        if !full_debug_info || vars.is_empty() && fallback_var.is_none() {
             return;
         }
 
-        // FIXME(eddyb) add debuginfo for unsized places too.
         let base = match local_ref {
-            LocalRef::Place(place) => place,
-            _ => return,
+            LocalRef::Operand(None) => return,
+
+            LocalRef::Operand(Some(operand)) => {
+                // "Spill" the value onto the stack, for debuginfo,
+                // without forcing non-debuginfo uses of the local
+                // to also load from the stack every single time.
+                // FIXME(#68817) use `llvm.dbg.value` instead,
+                // at least for the cases which LLVM handles correctly.
+                let spill_slot = PlaceRef::alloca(bx, operand.layout);
+                if let Some(name) = name {
+                    bx.set_var_name(spill_slot.llval, &(name + ".dbg.spill"));
+                }
+                operand.val.store(bx, spill_slot);
+                spill_slot
+            }
+
+            LocalRef::Place(place) => *place,
+
+            // FIXME(eddyb) add debuginfo for unsized places too.
+            LocalRef::UnsizedPlace(_) => return,
         };
 
         let vars = vars.iter().copied().chain(fallback_var);

src/test/codegen/naked-functions.rs

@@ -1,5 +1,3 @@
-// ignore-tidy-linelength
-
 // compile-flags: -C no-prepopulate-passes
 
 #![crate_type = "lib"]
@@ -61,19 +59,19 @@ pub fn naked_recursive() {
 
     naked_empty();
 
-    // CHECK-NEXT: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_return()
+    // CHECK-NEXT: %_4 = call i{{[0-9]+}} @naked_with_return()
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb2
     // CHECK: bb2:
 
-    // CHECK-NEXT: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_args_and_return(i{{[0-9]+}} %{{[0-9]+}})
+    // CHECK-NEXT: %_3 = call i{{[0-9]+}} @naked_with_args_and_return(i{{[0-9]+}} %_4)
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb3
     // CHECK: bb3:
 
-    // CHECK-NEXT: call void @naked_with_args(i{{[0-9]+}} %{{[0-9]+}})
+    // CHECK-NEXT: call void @naked_with_args(i{{[0-9]+}} %_3)
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb4

src/test/debuginfo/issue-12886.rs

@@ -6,7 +6,7 @@
 
 // gdb-command:run
 // gdb-command:next
-// gdb-check:[...]25[...]s
+// gdb-check:[...]24[...]let s = Some(5).unwrap(); // #break
 // gdb-command:continue
 
 #![feature(omit_gdb_pretty_printer_section)]