Merge pull request #2 from iximeow/address-frame-layout-comments

Resolve remaining frame layout comments

cranelift-codegen/src/ir/framelayout.rs

@@ -5,7 +5,7 @@ use crate::isa::RegUnit;
 use std::boxed::Box;
 
 #[cfg(not(feature = "std"))]
-use hashmap_core::HashMap;
+use crate::HashMap;
 #[cfg(feature = "std")]
 use std::collections::HashMap;
 
@@ -51,7 +51,7 @@ pub struct FrameLayout {
     /// Initial frame layout.
     pub initial: FrameLayoutChanges,
 
-    /// Instruction frame layout (changes). The map will not be dense,
+    /// Instruction frame layout (changes). Because the map will not be dense,
     /// a HashMap is used instead of a SecondaryMap.
     pub instructions: HashMap<Inst, FrameLayoutChanges>,
 }

cranelift-codegen/src/isa/x86/abi.rs

@@ -15,6 +15,7 @@ use crate::regalloc::RegisterSet;
 use crate::result::CodegenResult;
 use crate::stack_layout::layout_stack;
 use core::i32;
+use std::boxed::Box;
 use target_lexicon::{PointerWidth, Triple};
 
 /// Argument registers for x86-64
@@ -300,10 +301,26 @@ fn baldrdash_prologue_epilogue(func: &mut ir::Function, isa: &dyn TargetIsa) ->
     Ok(())
 }
 
+/// CFAState is `cranelift`'s model of the call frame layout at any particular point in a function.
+/// Changes in this layout are used to derive appropriate `ir::FrameLayoutChange` to record for
+/// relevant instructions.
 #[derive(Clone)]
 struct CFAState {
+    /// The register from which we can derive the call frame address. On x86_64, this is typically
+    /// `rbp`, but at function entry and exit may be `rsp` while the call frame is being
+    /// established.
     cf_ptr_reg: RegUnit,
+    /// Given that `cf_ptr_reg` is a register containing a pointer to some memory, `cf_ptr_offset`
+    /// is the offset from that pointer to the address of the start of this function's call frame.
+    ///
+    /// For a concrete x86_64 example, we will start this at 8 - the call frame begins immediately
+    /// before the return address. This will typically then be set to 16, after pushing `rbp` to
+    /// preserve the parent call frame. It is very unlikely the offset should be anything other
+    /// than one or two `usize`.
     cf_ptr_offset: isize,
+    /// The offset between the start of the call frame and the current stack pointer. This is
+    /// primarily useful to point to where on the stack preserved registers are, but is maintained
+    /// through the whole function for consistency.
     current_depth: isize,
 }
 
@@ -651,13 +668,11 @@ fn insert_common_epilogues(
         if let Some(inst) = pos.current_inst() {
             if pos.func.dfg[inst].opcode().is_return() {
                 // figure out if we need to insert end-of-function-aware frame layout information
-                let following_inst = pos.next_ebb().and_then(|next_ebb| {
-                    pos.goto_first_inst(next_ebb);
-                    pos.current_inst()
-                });
-
-                // and rewind back to where we want to insert prologue instructions
-                pos.goto_last_inst(ebb);
+                let following_inst = pos
+                    .func
+                    .layout
+                    .next_ebb(ebb)
+                    .and_then(|next_ebb| pos.func.layout.first_inst(next_ebb));
 
                 if following_inst.is_some() {
                     if let Some(ref mut frame_layout) = pos.func.frame_layout {
@@ -748,10 +763,12 @@ fn insert_common_epilogue(
             .instructions
             .entry(inst)
             .and_modify(|insts| {
-                let mut new_instructions = insts.to_vec();
-                new_instructions.push(new_cfa);
-                *insts = new_instructions.into_boxed_slice();
+                *insts = insts
+                    .into_iter()
+                    .map(|x| *x)
+                    .chain(std::iter::once(new_cfa))
+                    .collect::<Box<[_]>>();
             })
-            .or_insert_with(|| vec![new_cfa].into_boxed_slice());
+            .or_insert_with(|| Box::new([new_cfa]));
     }
 }