[turbofan] Spill rsi and rdi in their existing locations.

src/compiler/register-allocator.cc

Index: src/compiler/register-allocator.cc
diff --git a/src/compiler/register-allocator.cc b/src/compiler/register-allocator.cc
index 0dc76000f784771856dd10e29daedddb3b2ad27e..84c0b44a2cfcb5f4480ebac53c9aa9aa8f1cff41 100644
--- a/src/compiler/register-allocator.cc
+++ b/src/compiler/register-allocator.cc
@@ -692,7 +692,8 @@ TopLevelLiveRange::TopLevelLiveRange(int vreg, MachineType machine_type)
       spill_start_index_(kMaxInt),
       last_child_(this),
       last_pos_(nullptr),
-      splinter_(nullptr) {
+      splinter_(nullptr),
+      elidable_def_(nullptr) {
   bits_ |= SpillTypeField::encode(SpillType::kNoSpillType);
 }
 
@@ -1625,6 +1626,9 @@ void ConstraintBuilder::MeetConstraintsAfter(int instr_index) {
     if (temp->HasFixedPolicy()) AllocateFixed(temp, instr_index, false);
   }
   // Handle constant/fixed output operands.
+  bool is_prespilled_parameter =
+      first->arch_opcode() == ArchOpcode::kArchNop && first->OutputCount() == 2;

[Jarin, 2015/11/12 12:08:33]

This looks really brittle because we do not even check that both outputs define
the same vreg. Even then I would not like it because there might be legitimate
reasons for having two operands to ArchNop that have nothing to do with
pre-spilling (and we do not really want to special-case for specific opcodes
here).

[Mircea Trofin, 2015/11/12 18:01:35]

I see. I thought we used ArchNop just for introducing parameters. If that's not
the case, and we don't want to add another opcode (I'd also be weary of doing
that), then we're back to needing the tying info within the operand, which
basically removes most of the reasons for doing this design here, and favors the
previous one.

+
   for (size_t i = 0; i < first->OutputCount(); i++) {
     InstructionOperand* output = first->OutputAt(i);
     if (output->IsConstant()) {
@@ -1643,6 +1647,8 @@ void ConstraintBuilder::MeetConstraintsAfter(int instr_index) {
       UnallocatedOperand output_copy(UnallocatedOperand::ANY, output_vreg);
       bool is_tagged = code()->IsReference(output_vreg);
       AllocateFixed(first_output, instr_index, is_tagged);
+      MoveOperands* def = data()->AddGapMove(
+          instr_index + 1, Instruction::START, *first_output, output_copy);
 
       // This value is produced on the stack, we never need to spill it.
       if (first_output->IsStackSlot()) {
@@ -1651,9 +1657,19 @@ void ConstraintBuilder::MeetConstraintsAfter(int instr_index) {
         range->SetSpillOperand(LocationOperand::cast(first_output));
         range->SetSpillStartIndex(instr_index + 1);
         assigned = true;
+      } else if (is_prespilled_parameter) {
+        // we need the def so that the live range has the shape expected by
+        // the rest of the pipeline. Upon assigning operands, if this move
+        // ends up assigning to the stack slot the register value, we may
+        // delete it.

[Jarin, 2015/11/12 12:08:33]

To be honest, this MarkElidableDef business seems to be quite hacky.

I do not really understand why this is not a problem when we have a normal
fixed-register policy. Would not we then move to the spill slot twice? (Once
with the move on line 1650 and once on line 1677.)

[Mircea Trofin, 2015/11/12 18:01:35]

Here's what happens with a vanilla fixed register: the first move says initially
"v_x = [register]" (line 1650). Line 1677 only schedules a move.
OperandAssigner::CommitAssignment will take care of that. See the call to
TopLevelLiveRange::CommitSpillsAtDefinition from CommitAssignment. First, if
there is no spill operand, there won't be a move to a stack slot, so the move
inserted at line 1650 will either be to the same register, so that will be
elided later (==redundant), or to some other register, so not redundant.

If there are spills, and if the first segment of the live range chain (so the
top level leftover) is spilled, then CommitSpillsAtDefinition won't insert
another move (see the logic around "might_be_duplicated" in
CommitSpillsAtDefinition), and the move resulting from operand assignment
becomes the spill. If the top level isn't spilled, then the move at line 1650 is
to a register (see the paragraph above, same reasoning applies here). So we need
to spill at definition, which will happen in CommitSpillsAtDefinition because,
even if might_be_duplicated, we won't find the matching move.

In our case, we still need the move at line 1650 to define the virtual register.
After that, though, we need to somehow not spill at definition no matter what. 

Now that I think about it, maybe a better concept for the live range would be
"IsPreSpilled". Then, we could still schedule the spill at definition, but then
the logic would be to elide the move in CommitSpillsAtDefinition, instead of
adding a move (so "commit" means in this case "elided").

+        range->MarkElidableDef(def);
+        UnallocatedOperand* spill_op =
+            UnallocatedOperand::cast(first->OutputAt(1));
+        AllocateFixed(spill_op, instr_index, is_tagged);
+        range->SetSpillOperand(spill_op);
+        range->SetSpillStartIndex(instr_index + 1);
+        break;
       }
-      data()->AddGapMove(instr_index + 1, Instruction::START, *first_output,
-                         output_copy);
     }
     // Make sure we add a gap move for spilling (if we have not done
     // so already).
@@ -3016,6 +3032,11 @@ void OperandAssigner::CommitAssignment() {
       range->ConvertUsesToOperand(assigned, spill_operand);
     }
 
+    MoveOperands* move = top_range->elidable_def();
+    if (move != nullptr && move->destination().Equals(spill_operand)) {
+      move->Eliminate();
+    }
+
     if (!spill_operand.IsInvalid()) {
       // If this top level range has a child spilled in a deferred block, we use
       // the range and control flow connection mechanism instead of spilling at
@@ -3073,6 +3094,7 @@ void ReferenceMapPopulator::PopulateReferenceMaps() {
     if (!data()->IsReference(range)) continue;
     // Skip empty live ranges.
     if (range->IsEmpty()) continue;
+    if (range->IsReferenceAccountedByCaller()) continue;
 
     // Find the extent of the range and its children.
     int start = range->Start().ToInstructionIndex();