Optimise the deoptimisation check to improve performance on modern ARM cores....

src/arm/lithium-codegen-arm.cc

Index: src/arm/lithium-codegen-arm.cc
===================================================================
--- src/arm/lithium-codegen-arm.cc	(revision 7902)
+++ src/arm/lithium-codegen-arm.cc	(working copy)
@@ -85,6 +85,7 @@
   return GeneratePrologue() &&
       GenerateBody() &&
       GenerateDeferredCode() &&
+      GenerateDeoptJumpTable() &&
       GenerateSafepointTable();
 }
 
@@ -249,13 +250,39 @@
     __ jmp(code->exit());
   }
 
-  // Force constant pool emission at the end of deferred code to make
+  return !is_aborted();
+}
+
+
+bool LCodeGen::GenerateDeoptJumpTable() {
+  // Check that the jump table is accessible from everywhere in the function
+  // code, ie that offsets to the table can be encoded in the 24bit signed
+  // immediate of a branch instruction.
+  // To simplify we consider the code size from the first instruction to the
+  // end of the jump table. We also don't consider the pc load delta.
+  // Each entry in the jump table generates only one instruction.

[Søren Thygesen Gjesse, 2011/05/17 06:16:44]

I have not been able to find anywhere where we ensure that this assert holds. We
do try to not crankshaft compile large functions, so most likely this will never
happen. However I find that it will be better to just abort the crankshaft
compilation if this ever happens. That should mark the function as
not-optimizable, and we will not try to crankshaft compile it again.

[Alexandre, 2011/05/17 08:10:32]

Done: Changed the ASSERT to a test aborting the compilation if necesary.
On 2011/05/17 06:16:44, Søren Gjesse wrote:

+  ASSERT(is_int24((masm()->pc_offset() / Assembler::kInstrSize) +
+      deopt_jump_table_.length()));
+
+  __ RecordComment("[ Deoptimisation jump table");
+  Label table_start;
+  __ bind(&table_start);
+  for (int i = 0; i < deopt_jump_table_.length(); i++) {
+    __ bind(&deopt_jump_table_[i].label);
+    __ mov(pc, Operand(reinterpret_cast<int32_t>(deopt_jump_table_[i].address),
+                       RelocInfo::RUNTIME_ENTRY));
+  }
+  ASSERT(masm()->InstructionsGeneratedSince(&table_start) ==
+      deopt_jump_table_.length());
+  __ RecordComment("]");
+
+  // Force constant pool emission at the end of the jump table to make
   // sure that no constant pools are emitted after the official end of
   // the instruction sequence.
   masm()->CheckConstPool(true, false);
 
-  // Deferred code is the last part of the instruction sequence. Mark
-  // the generated code as done unless we bailed out.
+  // The deoptimization jump table is the last part of the instruction
+  // sequence. Mark the generated code as done unless we bailed out.
   if (!is_aborted()) status_ = DONE;
   return !is_aborted();
 }
@@ -595,19 +622,18 @@
     return;
   }
 
+  if (FLAG_trap_on_deopt) __ stop("trap_on_deopt");

[Søren Thygesen Gjesse, 2011/05/17 06:16:44]

Shouldn't this stop still be conditional if cc is not al?

[Alexandre, 2011/05/17 08:10:32]

It should. Done.

On 2011/05/17 06:16:44, Søren Gjesse wrote:

[Rodolph Perfetta, 2011/05/17 10:16:29]

Yes it should.

+
   if (cc == al) {
-    if (FLAG_trap_on_deopt) __ stop("trap_on_deopt");
     __ Jump(entry, RelocInfo::RUNTIME_ENTRY);
   } else {
-    if (FLAG_trap_on_deopt) {
-      Label done;
-      __ b(&done, NegateCondition(cc));
-      __ stop("trap_on_deopt");
-      __ Jump(entry, RelocInfo::RUNTIME_ENTRY);
-      __ bind(&done);
-    } else {
-      __ Jump(entry, RelocInfo::RUNTIME_ENTRY, cc);
+    // We often have several deopts to the same entry, reuse the last
+    // jump entry if this is the case.
+    if (deopt_jump_table_.is_empty() ||
+        (deopt_jump_table_.last().address != entry)) {
+      deopt_jump_table_.Add(JumpTableEntry(entry));
     }
+    __ b(cc, &deopt_jump_table_.last().label);
   }
 }