[v8-dev] Re: Consistently make the bounds check for AccessArgumentsAt explicit. (issue 11106012)

Mon, 15 Oct 2012 00:25:06 -0700 

Reviewers: Michael Starzinger,

Message:
As discussed offline, the "hot stuff" should be crankshafted, anyway, and this should use less instructions. In addition, the Lithium instruction can't deopt 
anymore, so this should be nicer to the register allocator.

What we would really need here is an optimization across Lithium operation
boundaries, so the cmp/branch/sub would be simplified to a sub/branch. Peephole 
optimizations, anyone...?

Landing...

Description:
Consistently make the bounds check for AccessArgumentsAt explicit.
This has the advantage that AccessArgumentsAt itself can't deopt anymore and the 
bounds check is visible for the elimination phase. Furthermore, things are
simply more consistent now, a good thing in itself. :-)


Please review this at http://codereview.chromium.org/11106012/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
  M src/arm/lithium-arm.cc
  M src/arm/lithium-codegen-arm.cc
  M src/hydrogen.cc
  M src/ia32/lithium-codegen-ia32.cc
  M src/ia32/lithium-ia32.cc
  M src/mips/lithium-codegen-mips.cc
  M src/mips/lithium-mips.cc
  M src/x64/lithium-codegen-x64.cc
  M src/x64/lithium-x64.cc


Index: src/arm/lithium-arm.cc
diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc
index e1ba04822ad26c45ef5b0592cc83b0cc1424de65..d513083c0f61d33180f9145fa18f5e9302eeede1 100644 
--- a/src/arm/lithium-arm.cc
+++ b/src/arm/lithium-arm.cc
@@ -2164,12 +2164,10 @@ LInstruction* LChunkBuilder::DoArgumentsObject(HArgumentsObject* instr) {
LInstruction* LChunkBuilder::DoAccessArgumentsAt(HAccessArgumentsAt* instr) { 
-  LOperand* arguments = UseRegister(instr->arguments());
+  LOperand* args = UseRegister(instr->arguments());
   LOperand* length = UseTempRegister(instr->length());
   LOperand* index = UseRegister(instr->index());
-  LAccessArgumentsAt* result =
-      new(zone()) LAccessArgumentsAt(arguments, length, index);
-  return AssignEnvironment(DefineAsRegister(result));
+ return DefineAsRegister(new(zone()) LAccessArgumentsAt(args, length, index)); 
 }


Index: src/arm/lithium-codegen-arm.cc
diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
index 3da16cdc177169974c816a66aed68a1663e70b87..d5b67a7a1ba65078c8eec836adc8967404dd6f05 100644 
--- a/src/arm/lithium-codegen-arm.cc
+++ b/src/arm/lithium-codegen-arm.cc
@@ -2904,14 +2904,9 @@ void LCodeGen::DoAccessArgumentsAt(LAccessArgumentsAt* instr) { 
   Register length = ToRegister(instr->length());
   Register index = ToRegister(instr->index());
   Register result = ToRegister(instr->result());
-
-  // Bailout index is not a valid argument index. Use unsigned check to get
-  // negative check for free.
-  __ sub(length, length, index, SetCC);
-  DeoptimizeIf(ls, instr->environment());
-
   // There are two words between the frame pointer and the last argument.
   // Subtracting from length accounts for one of them add one more.
+  __ sub(length, length, index);
   __ add(length, length, Operand(1));
   __ ldr(result, MemOperand(arguments, length, LSL, kPointerSizeLog2));
 }
Index: src/hydrogen.cc
diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 628d987a3408b28bca85d250c57d8dfaf710692e..374e54c97389e448721421ca80898aeb6d42df1d 100644 
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -9016,8 +9016,10 @@ void HGraphBuilder::GenerateArguments(CallRuntime* call) { 
   HInstruction* elements = AddInstruction(
       new(zone()) HArgumentsElements(false));
HInstruction* length = AddInstruction(new(zone()) HArgumentsLength(elements)); 
+  HInstruction* checked_index =
+      AddInstruction(new(zone()) HBoundsCheck(index, length));
   HAccessArgumentsAt* result =
-      new(zone()) HAccessArgumentsAt(elements, length, index);
+      new(zone()) HAccessArgumentsAt(elements, length, checked_index);
   return ast_context()->ReturnInstruction(result, call->id());
 }

Index: src/ia32/lithium-codegen-ia32.cc
diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 296fa324bedb1e077a3a50a2eadcb28c9936ee13..32c66a05f509df7df7d7314401f628dfbdcf4e87 100644 
--- a/src/ia32/lithium-codegen-ia32.cc
+++ b/src/ia32/lithium-codegen-ia32.cc
@@ -2742,12 +2742,9 @@ void LCodeGen::DoAccessArgumentsAt(LAccessArgumentsAt* instr) { 
   Register length = ToRegister(instr->length());
   Operand index = ToOperand(instr->index());
   Register result = ToRegister(instr->result());
-
-  __ sub(length, index);
-  DeoptimizeIf(below_equal, instr->environment());
-
   // There are two words between the frame pointer and the last argument.
   // Subtracting from length accounts for one of them add one more.
+  __ sub(length, index);
   __ mov(result, Operand(arguments, length, times_4, kPointerSize));
 }

Index: src/ia32/lithium-ia32.cc
diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc
index 38e7480d3df87c3222972c67dbb26ae1f590eb06..4b08a6dc68259b1056a0fae51394801c81d1bc0f 100644 
--- a/src/ia32/lithium-ia32.cc
+++ b/src/ia32/lithium-ia32.cc
@@ -2279,12 +2279,10 @@ LInstruction* LChunkBuilder::DoArgumentsObject(HArgumentsObject* instr) {
LInstruction* LChunkBuilder::DoAccessArgumentsAt(HAccessArgumentsAt* instr) { 
-  LOperand* arguments = UseRegister(instr->arguments());
+  LOperand* args = UseRegister(instr->arguments());
   LOperand* length = UseTempRegister(instr->length());
   LOperand* index = Use(instr->index());
-  LAccessArgumentsAt* result =
-      new(zone()) LAccessArgumentsAt(arguments, length, index);
-  return AssignEnvironment(DefineAsRegister(result));
+ return DefineAsRegister(new(zone()) LAccessArgumentsAt(args, length, index)); 
 }


Index: src/mips/lithium-codegen-mips.cc
diff --git a/src/mips/lithium-codegen-mips.cc b/src/mips/lithium-codegen-mips.cc index 6488285c3b16ff4861d3c3b3d8d45524d1ad908a..77d4b9b09665e7f793ffef98394890489778ee60 100644 
--- a/src/mips/lithium-codegen-mips.cc
+++ b/src/mips/lithium-codegen-mips.cc
@@ -2612,14 +2612,6 @@ void LCodeGen::DoAccessArgumentsAt(LAccessArgumentsAt* instr) { 
   Register length = ToRegister(instr->length());
   Register index = ToRegister(instr->index());
   Register result = ToRegister(instr->result());
-
-  // Bailout index is not a valid argument index. Use unsigned check to get
-  // negative check for free.
-
- // TODO(plind): Shoud be optimized to do the sub before the DeoptimizeIf(), 
-  // as they do in Arm. It will save us an instruction.
-  DeoptimizeIf(ls, instr->environment(), length, Operand(index));
-
   // There are two words between the frame pointer and the last argument.
   // Subtracting from length accounts for one of them, add one more.
   __ subu(length, length, index);
Index: src/mips/lithium-mips.cc
diff --git a/src/mips/lithium-mips.cc b/src/mips/lithium-mips.cc
index 62392b4a5f6ea0cf2969bcc87ae7b4b7ae2cd461..4015bffba3fc6dd9a37506ea09a8d9444d73914e 100644 
--- a/src/mips/lithium-mips.cc
+++ b/src/mips/lithium-mips.cc
@@ -2106,12 +2106,10 @@ LInstruction* LChunkBuilder::DoArgumentsObject(HArgumentsObject* instr) {
LInstruction* LChunkBuilder::DoAccessArgumentsAt(HAccessArgumentsAt* instr) { 
-  LOperand* arguments = UseRegister(instr->arguments());
+  LOperand* args = UseRegister(instr->arguments());
   LOperand* length = UseTempRegister(instr->length());
   LOperand* index = UseRegister(instr->index());
-  LAccessArgumentsAt* result =
-      new(zone()) LAccessArgumentsAt(arguments, length, index);
-  return AssignEnvironment(DefineAsRegister(result));
+ return DefineAsRegister(new(zone()) LAccessArgumentsAt(args, length, index)); 
 }


Index: src/x64/lithium-codegen-x64.cc
diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc
index fea0afa7842c2a784a97cb7b2f94ee5ff43a059d..c08ca7b78cf4caf4018ce23dbd2b97e1f30f55fb 100644 
--- a/src/x64/lithium-codegen-x64.cc
+++ b/src/x64/lithium-codegen-x64.cc
@@ -2601,16 +2601,13 @@ void LCodeGen::DoAccessArgumentsAt(LAccessArgumentsAt* instr) { 
   Register arguments = ToRegister(instr->arguments());
   Register length = ToRegister(instr->length());
   Register result = ToRegister(instr->result());
-
+  // There are two words between the frame pointer and the last argument.
+  // Subtracting from length accounts for one of them add one more.
   if (instr->index()->IsRegister()) {
     __ subl(length, ToRegister(instr->index()));
   } else {
     __ subl(length, ToOperand(instr->index()));
   }
-  DeoptimizeIf(below_equal, instr->environment());
-
-  // There are two words between the frame pointer and the last argument.
-  // Subtracting from length accounts for one of them add one more.
__ movq(result, Operand(arguments, length, times_pointer_size, kPointerSize)); 
 }

Index: src/x64/lithium-x64.cc
diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc
index dd3054f2da620dd245db260826510a6d9b69d940..add56762531417ef58dc2996978286db7402811d 100644 
--- a/src/x64/lithium-x64.cc
+++ b/src/x64/lithium-x64.cc
@@ -2167,12 +2167,10 @@ LInstruction* LChunkBuilder::DoArgumentsObject(HArgumentsObject* instr) {
LInstruction* LChunkBuilder::DoAccessArgumentsAt(HAccessArgumentsAt* instr) { 
-  LOperand* arguments = UseRegister(instr->arguments());
+  LOperand* args = UseRegister(instr->arguments());
   LOperand* length = UseTempRegister(instr->length());
   LOperand* index = Use(instr->index());
-  LAccessArgumentsAt* result =
-      new(zone()) LAccessArgumentsAt(arguments, length, index);
-  return AssignEnvironment(DefineAsRegister(result));
+ return DefineAsRegister(new(zone()) LAccessArgumentsAt(args, length, index)); 
 }




--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to