Merge pull request #491 from rustcoreutils/updates

[cc] Optimization fixes

Author: jgarzik

cc/Cargo.toml

@@ -16,6 +16,10 @@ plib = { path = "../plib" }
 [dev-dependencies]
 tempfile = "3"
 
+[features]
+# Enable full compile matrix (4 configs) instead of default single config (-O -g)
+test_matrix = []
+
 [lib]
 name = "posixutils_cc"
 path = "./lib.rs"

cc/arch/aarch64/codegen.rs

@@ -1894,6 +1894,8 @@ impl Aarch64CodeGen {
         self.handle_call_return_value(insn, types, frame_size);
     }
 
+    /// Emit a select (ternary) instruction using CSEL
+    /// This is used for pure ternary expressions: cond ? a : b
     fn emit_select(&mut self, insn: &Instruction, frame_size: i32) {
         let (cond, then_val, else_val) = match (insn.src.first(), insn.src.get(1), insn.src.get(2))
         {
@@ -1930,7 +1932,7 @@ impl Aarch64CodeGen {
         self.emit_move(then_val, then_reg, size, frame_size);
         self.emit_move(else_val, else_reg, size, frame_size);
 
-        // LIR: compare condition with zero
+        // Compare condition with zero
         self.push_lir(Aarch64Inst::Cmp {
             size: OperandSize::B64,
             src1: cond_reg,

cc/arch/aarch64/expression.rs

@@ -307,7 +307,7 @@ impl Aarch64CodeGen {
             Opcode::Zext => {
                 // Zero extend: use uxtb, uxth, or just mov for 32->64
                 self.emit_move(src, dst_reg, 64, frame_size);
-                match insn.size {
+                match insn.src_size {
                     8 => {
                         self.push_lir(Aarch64Inst::Uxtb {
                             src: dst_reg,

cc/arch/x86_64/codegen.rs

@@ -1850,6 +1850,8 @@ impl X86_64CodeGen {
         self.handle_call_return_value(insn, types);
     }
 
+    /// Emit a select (ternary) instruction using CMOVcc
+    /// This is used for pure ternary expressions: cond ? a : b
     fn emit_select(&mut self, insn: &Instruction) {
         let (cond, then_val, else_val) = match (insn.src.first(), insn.src.get(1), insn.src.get(2))
         {
@@ -1867,54 +1869,63 @@ impl X86_64CodeGen {
             Loc::Reg(r) => *r,
             _ => Reg::R10, // Use scratch register R10
         };
+
+        // Move else value into destination first (default if condition is false)
         self.emit_move(else_val, dst_reg, size);
+
+        // Test condition
         let cond_loc = self.get_location(cond);
         match &cond_loc {
             Loc::Reg(r) => {
-                // LIR: test register with itself
+                // Test register with itself
                 self.push_lir(X86Inst::Test {
                     size: OperandSize::B64,
                     src: GpOperand::Reg(*r),
                     dst: GpOperand::Reg(*r),
                 });
             }
             Loc::Imm(v) => {
+                // Constant condition - just use appropriate value
                 if *v != 0 {
                     self.emit_move(then_val, dst_reg, size);
                     if !matches!(&dst_loc, Loc::Reg(r) if *r == dst_reg) {
                         self.emit_move_to_loc(dst_reg, &dst_loc, size);
                     }
                     return;
                 }
+                // else_val already in dst_reg
                 if !matches!(&dst_loc, Loc::Reg(r) if *r == dst_reg) {
                     self.emit_move_to_loc(dst_reg, &dst_loc, size);
                 }
                 return;
             }
             _ => {
+                // Load condition to scratch register and test
                 self.emit_move(cond, Reg::R11, 64);
-                // LIR: test R11 with itself
                 self.push_lir(X86Inst::Test {
                     size: OperandSize::B64,
                     src: GpOperand::Reg(Reg::R11),
                     dst: GpOperand::Reg(Reg::R11),
                 });
             }
         }
+
+        // Conditional move: if condition is non-zero (NE), use then_val
         // Use R11 for then_val when dst_reg is R10 to avoid clobbering else value
         let then_reg = if dst_reg == Reg::R10 {
             Reg::R11
         } else {
             Reg::R10
         };
         self.emit_move(then_val, then_reg, size);
-        // LIR: conditional move if not equal (non-zero)
         self.push_lir(X86Inst::CMov {
             cc: CondCode::Ne,
             size: op_size,
             src: GpOperand::Reg(then_reg),
             dst: dst_reg,
         });
+
+        // Move to final destination if needed
         if !matches!(&dst_loc, Loc::Reg(r) if *r == dst_reg) {
             self.emit_move_to_loc(dst_reg, &dst_loc, size);
         }

cc/arch/x86_64/expression.rs

@@ -397,7 +397,12 @@ impl X86_64CodeGen {
         };
         match insn.op {
             Opcode::Zext => {
-                self.emit_move(src, dst_reg, insn.size);
+                // Move source at its original size - this ensures we only load
+                // the valid bits from stack/memory. On x86-64, 32-bit register
+                // writes automatically zero the upper 32 bits.
+                // For 8/16-bit sources, emit_move uses movzbl/movzwl which
+                // zero-extends to 32 bits (and thus to 64 bits).
+                self.emit_move(src, dst_reg, insn.src_size.max(32));
             }
             Opcode::Sext => {
                 // Move source at its original size, then sign-extend

cc/arch/x86_64/regalloc.rs

@@ -260,9 +260,10 @@ pub fn opcode_constraints(op: Opcode) -> RegConstraints {
             inputs: &[Reg::Rax],
         },
         // Variable shifts: count must be in Cl (Rcx)
-        // Note: shifts don't clobber Rcx, they just require it as input
+        // The codegen moves the shift count INTO Rcx, clobbering it.
+        // For immediate shifts, this constraint is conservative but safe.
         Opcode::Shl | Opcode::Lsr | Opcode::Asr => RegConstraints {
-            clobbers: &[],
+            clobbers: &[Reg::Rcx],
             inputs: &[Reg::Rcx],
         },
         // VaArg: emit_va_arg_int uses Rax for reg_save_area/overflow pointer,

cc/ir/dce.rs

@@ -64,6 +64,7 @@ fn is_root(op: Opcode) -> bool {
             | Opcode::Alloca
             | Opcode::Setjmp  // Has side effects (saves context)
             | Opcode::Longjmp // Never returns (noreturn)
+            | Opcode::Asm // Inline assembly has side effects
     )
 }
 
@@ -91,20 +92,30 @@ fn get_uses(insn: &Instruction) -> Vec<PseudoId> {
         uses.push(indirect);
     }
 
+    // Inline assembly inputs (the pseudos that the asm reads)
+    if let Some(ref asm_data) = insn.asm_data {
+        for input in &asm_data.inputs {
+            uses.push(input.pseudo);
+        }
+    }
+
     uses
 }
 
-/// Find the instruction that defines a pseudo.
-/// Returns (block_index, instruction_index) if found.
-fn find_def(func: &Function, id: PseudoId) -> Option<(usize, usize)> {
+/// Find all instructions that define a pseudo.
+/// Returns vec of (block_index, instruction_index) for each definition.
+/// After inlining, a pseudo may have multiple definitions from different branches
+/// (e.g., the return target is written to from multiple return paths).
+fn find_all_defs(func: &Function, id: PseudoId) -> Vec<(usize, usize)> {
+    let mut defs = Vec::new();
     for (bb_idx, bb) in func.blocks.iter().enumerate() {
         for (insn_idx, insn) in bb.insns.iter().enumerate() {
             if insn.target == Some(id) {
-                return Some((bb_idx, insn_idx));
+                defs.push((bb_idx, insn_idx));
             }
         }
     }
-    None
+    defs
 }
 
 /// Eliminate dead code using mark-sweep algorithm.
@@ -128,8 +139,9 @@ fn eliminate_dead_code(func: &mut Function) -> bool {
 
     // Phase 2: Propagate liveness transitively
     while let Some(id) = worklist.pop_front() {
-        // Find the instruction that defines this pseudo
-        if let Some((bb_idx, insn_idx)) = find_def(func, id) {
+        // Find all instructions that define this pseudo
+        // (there may be multiple after inlining, e.g., return target written from multiple paths)
+        for (bb_idx, insn_idx) in find_all_defs(func, id) {
             let insn = &func.blocks[bb_idx].insns[insn_idx];
 
             // Mark all operands of the defining instruction as live

cc/ir/dominate.rs

@@ -149,7 +149,11 @@ pub fn domtree_build(func: &mut Function) {
             // Find new idom as intersection of all processed predecessors
             let mut new_idom: Option<usize> = None;
             for parent_id in parents {
-                let parent_nr = postorder_nr[&parent_id];
+                // Skip predecessors that weren't reached during DFS (unreachable blocks)
+                let parent_nr = match postorder_nr.get(&parent_id) {
+                    Some(&nr) => nr,
+                    None => continue,
+                };
                 if doms[parent_nr].is_none() {
                     continue;
                 }
@@ -634,4 +638,62 @@ mod tests {
         assert!(func.dominates(BasicBlockId(3), BasicBlockId(4)));
         assert!(!func.dominates(BasicBlockId(3), BasicBlockId(1)));
     }
+
+    /// Test that domtree_build handles unreachable blocks gracefully.
+    /// Unreachable blocks have predecessors that weren't visited during DFS.
+    #[test]
+    fn test_domtree_with_unreachable_block() {
+        // Create a CFG with an unreachable block:
+        //       entry(0)
+        //          |
+        //          v
+        //        bb1(1)
+        //          |
+        //          v
+        //        exit(2)
+        //
+        //    unreachable(3) <-- has edge from nowhere reachable
+        //          |
+        //          v
+        //        bb1(1) <-- unreachable points TO bb1, making bb1 have unreachable as predecessor
+
+        let types = TypeTable::new(64);
+        let mut func = Function::new("test", types.void_id);
+
+        let mut entry = BasicBlock::new(BasicBlockId(0));
+        entry.children = vec![BasicBlockId(1)];
+        entry.add_insn(Instruction::new(Opcode::Entry));
+        entry.add_insn(Instruction::br(BasicBlockId(1)));
+
+        let mut bb1 = BasicBlock::new(BasicBlockId(1));
+        // bb1 has both entry AND unreachable as predecessors
+        bb1.parents = vec![BasicBlockId(0), BasicBlockId(3)];
+        bb1.children = vec![BasicBlockId(2)];
+        bb1.add_insn(Instruction::br(BasicBlockId(2)));
+
+        let mut exit = BasicBlock::new(BasicBlockId(2));
+        exit.parents = vec![BasicBlockId(1)];
+        exit.add_insn(Instruction::ret(None));
+
+        // Unreachable block - not reachable from entry
+        let mut unreachable = BasicBlock::new(BasicBlockId(3));
+        unreachable.children = vec![BasicBlockId(1)];
+        unreachable.add_insn(Instruction::br(BasicBlockId(1)));
+
+        func.entry = BasicBlockId(0);
+        func.blocks = vec![entry, bb1, exit, unreachable];
+
+        // This should not panic - the fix skips predecessors not reached during DFS
+        domtree_build(&mut func);
+
+        // Verify the reachable blocks have correct dominators
+        let entry_block = func.get_block(BasicBlockId(0)).unwrap();
+        assert!(entry_block.idom.is_none());
+
+        let bb1_block = func.get_block(BasicBlockId(1)).unwrap();
+        assert_eq!(bb1_block.idom, Some(BasicBlockId(0)));
+
+        let exit_block = func.get_block(BasicBlockId(2)).unwrap();
+        assert_eq!(exit_block.idom, Some(BasicBlockId(1)));
+    }
 }