[v8-dev] Re: [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

On 2015/09/17 15:37:55, torbjorng wrote:

This CL apparently triggers BUG=532969.


Opps, sorry. Fix up for review at  
https://codereview.chromium.org/1351943002/.


https://codereview.chromium.org/1323463005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

Benedikt, all the ports are now complete - could you review / stamp please?


On 2015/09/10 20:14:06, akos.palfi.imgtec wrote:

Hi Ross - I've uploaded the mips ports:
https://codereview.chromium.org/1334873002


Thanks for the port - now integrated.

However, there's a test failure (cctest/test-interpreter/InterpreterCall)  
that

I

haven't investigated deeply, since it fails on all arch's.


Turns out this was specific to when the callee function was compiled using
turbofan (only happened with flags --always-opt and --turbo), in which case  
it

would return a HeapNumber instead of a Smi. Fixed by making the test use
SameValue instead of a SMI equality check.


https://codereview.chromium.org/1323463005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

Benedikt: I'd like to port this to the other architectures. Before I do so  
could

you have a look and let me know if you are happy with the general approach?
Thanks.

https://codereview.chromium.org/1323463005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Continuing removing deprecated functions from cctests (issue 1331013003 by myth...@google.com)

[rmcilroy]

Looks good overall, just a couple of nits.

Jochen, do you want to give it a once over too (If you are fine with the
approach I'll not bother you with future ones).


https://codereview.chromium.org/1331013003/diff/1/test/cctest/test-unscopables-hidden-prototype.cc
File test/cctest/test-unscopables-hidden-prototype.cc (right):

https://codereview.chromium.org/1331013003/diff/1/test/cctest/test-unscopables-hidden-prototype.cc#newcode29
test/cctest/test-unscopables-hidden-prototype.cc:29:
v8::Local curr_context = isolate->GetCurrentContext();
nit - please don't abbreviate - current_context.

https://codereview.chromium.org/1331013003/diff/1/test/cctest/test-unscopables-hidden-prototype.cc#newcode45
test/cctest/test-unscopables-hidden-prototype.cc:45:
object->SetPrototype(curr_context, hidden_prototype).FromMaybe(false);
Could you add a CHECK() around this to check the value returned is true.

https://codereview.chromium.org/1331013003/diff/1/test/cctest/test-unscopables-hidden-prototype.cc#newcode62
test/cctest/test-unscopables-hidden-prototype.cc:62: .FromMaybe(-1));
nit - could .FromMaybe(-1) be on the same line as ->Int32Value couldn't
it (or did git cl format do this to you)?

https://codereview.chromium.org/1331013003/diff/1/test/cctest/trace-extension.cc
File test/cctest/trace-extension.cc (right):

https://codereview.chromium.org/1331013003/diff/1/test/cctest/trace-extension.cc#newcode53
test/cctest/trace-extension.cc:53: .FromMaybe(false)) {
nit - could .FromMaybe(..) go on the same line as .ToLocalChecked() here
too?

https://codereview.chromium.org/1331013003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Continuing removing deprecated functions from cctests (issue 1331013003 by myth...@google.com)

[rmcilroy]

can you please run tryjobs?


I kicked these for Mythri (I don't think she has permission yet).

https://codereview.chromium.org/1331013003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

Looks really great. All nits except for the comment about TwoParameterTest,  
but

the rest lgtm!


https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode18
src/compiler/bytecode-graph-builder.cc:18: // - Need story for context
parameter, closure parameter, this.
nit - I think only closure parameter is relevent here any longer (we
have a story for context parameter and this)

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode137
src/compiler/bytecode-graph-builder.cc:137: int register_count =
bytecode_array()->register_count();
nit - you could just inline bytecode_array()->register_count() in the
constructor call below (like you do with parameter_count).

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode144
src/compiler/bytecode-graph-builder.cc:144: UNIMPLEMENTED();  // write
ast-graph-builder equivalent.
nit - TODO(oth): Write...

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode193
src/compiler/bytecode-graph-builder.cc:193: //  Node* node =
jsgraph()->Int32Constant(iterator.GetSmi8Operand(0));
remove comment

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.cc#newcode321
src/compiler/bytecode-graph-builder.cc:321:
You'll need to declare VisitStoreIC and VisitKeyedStoreIC now too (as
UNIMPLEMENTED()).

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.h#newcode155
src/compiler/bytecode-graph-builder.h:155: int register_base_;
nit - fields should be at the end (after private methods).

https://codereview.chromium.org/1291693004/diff/260001/src/compiler/bytecode-graph-builder.h#newcode166
src/compiler/bytecode-graph-builder.h:166: int
RegisterToValuesIndex(interpreter::Register the_register) const;
nit - put this above the accessors

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67
src/interpreter/bytecode-array-iterator.cc:67: return
FixedArray::get(constants, GetIndexOperand(operand_index));
this should be "constants->get(GetIndexOperand(operand_index));" I
think.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/test-run-bytecode-graph-builder.cc
File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode65
test/cctest/compiler/test-run-bytecode-graph-builder.cc:65:
i::FLAG_ignition = true;
need to also add i::FLAG_vector_stores now.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode187
test/cctest/compiler/test-run-bytecode-graph-builder.cc:187:
Object::ToString(isolate, param2.second).ToHandleChecked()->ToCString();
This seems overly complicated. You don't actually need to pass the same
values to the code snippet as you pass during the actual call. The
values you pass to the code snippet are just what is going to be used
during the "compileRun" to produce the bytecode, so just passing '(0,
0)' for both would be fine.

I think without this you can get rid of the TwoParameterTest and just
use code snippets with an extra field for parameters - i.e.: add the
field "Handle parameters[2]" to ExpectedSnippet and do:

size_t num_snippets = sizeof(snippets) / sizeof(snippets[0]);
  for (size_t i = 0; i < num_snippets; i++) {
ScopedVector script(1024);
SNPrintF(script, "function %s(p1, p2) { %s }\n%s(0, 0);",
kFunctionName,
 snippets[i].code_snippet, kFunctionName);

BytecodeGraphTester tester(handles.main_isolate(),
handles.main_zone(),
   script.start());
auto callable = tester.GetCallable();
Handle return_val = callable(snippet[i].param[0],
snippet[i].param[1]).ToHandleChecked();
CHECK(return_val.is_identical_to(snippets[i].return_val));
  }

Grab me in person if any of this doesn't make sense.

https://codereview.chromium.org/1291693004/diff/260001/test/cctest/compiler/test-run-bytecode-graph-builder.cc#newcode258
test/cctest/compiler/test-run-bytecode-graph-builder.cc:258: }
nit - could you add a TODO to add a test for constants (e.g, snippets
like "return "test_string";" and "return 0.2" etc.

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev

[v8-dev] Re: Continuing removing deprecated functions from cctests (issue 1331013003 by myth...@google.com)

[rmcilroy]

lgtm, thanks!

https://codereview.chromium.org/1331013003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Continuing removing deprecated functions from cctests (issue 1331013003 by myth...@google.com)

[rmcilroy]

https://codereview.chromium.org/1331013003/diff/1/test/cctest/test-unscopables-hidden-prototype.cc
File test/cctest/test-unscopables-hidden-prototype.cc (right):

https://codereview.chromium.org/1331013003/diff/1/test/cctest/test-unscopables-hidden-prototype.cc#newcode62
test/cctest/test-unscopables-hidden-prototype.cc:62: .FromMaybe(-1));
If this is what git cl format wants then I'm fine with it :).

https://codereview.chromium.org/1331013003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

Looks great, let's land it!


https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67
src/interpreter/bytecode-array-iterator.cc:67: return
FixedArray::get(constants, GetIndexOperand(operand_index));
On 2015/09/10 13:59:06, oth wrote:

On 2015/09/10 10:19:43, rmcilroy wrote:
> this should be "constants->get(GetIndexOperand(operand_index));" I

think.


The static version returns a handle for the element that this method

returns.

The member version returns bare object pointer. The current path might

be

cleaner than cons-ing up handle here. WDYT?


Ahh you're right sorry. Strange that this is a static method though...
but not your problem :).

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

Looks great, let's land it!


https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/260001/src/interpreter/bytecode-array-iterator.cc#newcode67
src/interpreter/bytecode-array-iterator.cc:67: return
FixedArray::get(constants, GetIndexOperand(operand_index));
On 2015/09/10 13:59:06, oth wrote:

On 2015/09/10 10:19:43, rmcilroy wrote:
> this should be "constants->get(GetIndexOperand(operand_index));" I

think.


The static version returns a handle for the element that this method

returns.

The member version returns bare object pointer. The current path might

be

cleaner than cons-ing up handle here. WDYT?


Ahh you're right sorry. Strange that this is a static method though...
but not your problem :).

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

Thanks for the reviews, I'll port to ia32, arm and arm64 now.

+v8-mips-ports: Could you port for mips/mips64 please?


https://codereview.chromium.org/1323463005/diff/60001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1323463005/diff/60001/src/interpreter/bytecode-array-builder.h#newcode71
src/interpreter/bytecode-array-builder.h:71: // |callable|, the receiver
should be in |receiver| and all subsiquent
On 2015/09/10 14:17:27, oth wrote:

nit s/subsiquent/subsequent/


Done.

https://codereview.chromium.org/1323463005/diff/60001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1323463005/diff/60001/src/x64/builtins-x64.cc#newcode1795
src/x64/builtins-x64.cc:1795: __
Call(masm->isolate()->builtins()->Call(), RelocInfo::CODE_TARGET);
On 2015/09/10 09:51:34, Benedikt Meurer wrote:

Hm, I think this should be Jump instead of Call+ret


As discussed offline, this requires popping and repushing the return
address on x64. Done.

https://codereview.chromium.org/1323463005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for property store operations. (issue 1319833004 by rmcil...@chromium.org)

[rmcilroy]

Ah, you can ignore my comment about SLOPPY/STRICT because I see elsewhere  
that

you are only handling SLOPPY for now.


Yes, we only handle SLOPPY mode for now. As discussed offline we will  
probably
have to have Strict/Strong versions of these bytecodes, but I'll do that in  
a

future CL. Thanks for the reviews.

https://codereview.chromium.org/1319833004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for property store operations. (issue 1319833004 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1319833004/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1319833004/diff/1/src/interpreter/bytecodes.h#newcode101
src/interpreter/bytecodes.h:101: int index() const { return index_; }
On 2015/09/09 14:37:29, oth wrote:

Could fail fast here - DCHECK(is_valid(index())).


Done.

https://codereview.chromium.org/1319833004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Ensure that implicit return undefined is generated. (issue 1308693014 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: oth, Michael Starzinger,

Message:
Orion, please have a look, thanks.

Michi for owner stamp (comments welcome)

Description:
[Interpreter] Ensure that implicit return undefined is generated.

When there is no explicit return we need to generate an implicit
return undefined.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1308693014/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+17, -0 lines):
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecode-generator.cc
  M test/cctest/interpreter/test-bytecode-generator.cc


Index: src/interpreter/bytecode-array-builder.cc
diff --git a/src/interpreter/bytecode-array-builder.cc  
b/src/interpreter/bytecode-array-builder.cc
index  
dba816d61810d63259412f946f219494c2e134ef..5f97bce75b2f34184e3cd99753aebe084317169a  
100644

--- a/src/interpreter/bytecode-array-builder.cc
+++ b/src/interpreter/bytecode-array-builder.cc
@@ -37,6 +37,12 @@ void BytecodeArrayBuilder::set_parameter_count(int  
number_of_parameters) {
 int BytecodeArrayBuilder::parameter_count() const { return  
parameter_count_; }



+bool BytecodeArrayBuilder::HasExplicitReturn() {
+  return !bytecodes_.empty() &&
+ bytecodes_.back() == Bytecodes::ToByte(Bytecode::kReturn);
+}
+
+
 Register BytecodeArrayBuilder::Parameter(int parameter_index) {
   DCHECK_GE(parameter_index, 0);
   DCHECK_LT(parameter_index, parameter_count_);
Index: src/interpreter/bytecode-array-builder.h
diff --git a/src/interpreter/bytecode-array-builder.h  
b/src/interpreter/bytecode-array-builder.h
index  
d4e1c34e540a2d67a22c19d61c7372c78dbf6cfb..a50c28b0ebe875252ea8ff68e34c0fdf58c1e67f  
100644

--- a/src/interpreter/bytecode-array-builder.h
+++ b/src/interpreter/bytecode-array-builder.h
@@ -35,6 +35,9 @@ class BytecodeArrayBuilder {
   void set_locals_count(int number_of_locals);
   int locals_count() const;

+  // Returns true if the bytecode has an explicit return at the end.
+  bool HasExplicitReturn();
+
   Register Parameter(int parameter_index);

   // Constant loads to accumulator.
Index: src/interpreter/bytecode-generator.cc
diff --git a/src/interpreter/bytecode-generator.cc  
b/src/interpreter/bytecode-generator.cc
index  
487b86543794b9effc862db130ea37a2e0e36ca7..0f24995c92a9b150b38f002a73ea5d70ff61a360  
100644

--- a/src/interpreter/bytecode-generator.cc
+++ b/src/interpreter/bytecode-generator.cc
@@ -45,6 +45,13 @@ Handle  
BytecodeGenerator::MakeBytecode(CompilationInfo* info) {

   // Visit statements in the function body.
   VisitStatements(info->literal()->body());

+  // If the last bytecode wasn't a return, then return 'undefined' to avoid
+  // falling off the end.
+  if (!builder_.HasExplicitReturn()) {
+builder_.LoadUndefined();
+builder_.Return();
+  }
+
   set_scope(nullptr);
   set_info(nullptr);
   return builder_.ToBytecodeArray();
Index: test/cctest/interpreter/test-bytecode-generator.cc
diff --git a/test/cctest/interpreter/test-bytecode-generator.cc  
b/test/cctest/interpreter/test-bytecode-generator.cc
index  
c62e5efff9f2a93756f4022d64469b85d4f1f252..25a71488936bab3c9ecb5f1454990395a02bbd7c  
100644

--- a/test/cctest/interpreter/test-bytecode-generator.cc
+++ b/test/cctest/interpreter/test-bytecode-generator.cc
@@ -79,6 +79,7 @@ TEST(PrimitiveReturnStatements) {
   BytecodeGeneratorHelper helper;

   ExpectedSnippet snippets[] = {
+  {"", 0, 1, 2, {B(LdaUndefined), B(Return)}, 0},
   {"return;", 0, 1, 2, {B(LdaUndefined), B(Return)}, 0},
   {"return null;", 0, 1, 2, {B(LdaNull), B(Return)}, 0},
   {"return true;", 0, 1, 2, {B(LdaTrue), B(Return)}, 0},


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for property store operations. (issue 1319833004 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: oth, Michael Starzinger, mvstanton,

Message:
Michi / Orion, please review interpreter parts
Michael for vector stuff.
Thanks!

Description:
[Interpreter] Add support for property store operations.

Adds support for property store operations via Store/KeyedStore ICs. Adds  
the

following bytecodes:
 - StoreIC
 - KeyedStoreIC

The --vector_store flag is now required for --ignition.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1319833004/

Base URL: https://chromium.googlesource.com/v8/v8.git@int_implicit_ret

Affected files (+407, -39 lines):
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/flag-definitions.h
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecode-generator.cc
  M src/interpreter/bytecodes.h
  M src/interpreter/interpreter.h
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-bytecode-generator.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/interpreter/bytecode-array-builder-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Ensure that implicit return undefined is generated. (issue 1308693014 by rmcil...@chromium.org)

[rmcilroy]

I am not an owner of anything in this CL, but I am happy to
offer my thoughts
...


Sure, but Orion is not a committer yet and I need a committer stamp :). Your
thoughts are much appreciated!


https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-generator.cc
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1308693014/diff/1/src/interpreter/bytecode-generator.cc#newcode50
src/interpreter/bytecode-generator.cc:50: if
(!builder_.HasExplicitReturn()) {
On 2015/09/08 08:25:10, Michael Starzinger wrote:

This looks dangerous. Depending on how control flow is modeled there

could very

well be a case where the last instruction in the byte-code stream is a

return

statement, but it is not guaranteed post-dominate all executions

paths. Consider

the following ...



function f(a) {
   if (a) { return 23; }
}


Good point, we need to take this into account, but we would need to take
this into account once we start adding control flow in any case since
the if block will need to jump to an actual bytecode, and the bytecode
array builder will need to back-patch the jump instruction with the
target of the jump.

The last thing the bytecode-generator would do in the graph above would
be to try and back-patch the if jump to target the next bytecode which
is to be emitted. Since that bytecode doesn't exist yet we could easily
spot in the bytecode-array-builder that there is a jump instruction
which is patched to a non-existent bytecode (e.g., just having a field
tracking the greatest bytecode indice which was patched into a jump
instruction and check if that is greater-than/equal than
bytecodes_.size()), and add that to the HasExplicitReturn conditions?
WDYT?

https://codereview.chromium.org/1308693014/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

Ok. Doing the whole handler as native code would add a whole lot more  
complexity
(e.g., ensuring that interpreter are saved and restored properly). Instead,  
I've
written a small builtin called PushArgsAndCall which does the pushing and  
then
directly calls builtin::Call. This avoids the need for CallVarArgs. How's  
this?




And especially, since you might want to avoid leaving and reentering the
interpreter on calls between bytecode functions, you'll probably need to
do some frame magic in the Call bytecode handler anyway (which TF cannot
support in general).


I don't think we will want to do this - there is very little code in
builtin::EnterInterpreter which we can avoid doing even if we are already  
in the
interpreter, so it probably isn't worth special casing  
interpreter->interpreter

calls.


So my feeling here that this is trying to crack a walnut
with a sledgehammer. But I that is just my feeling; I don't want to block  
the

interpreter, which means this is not a N-O-T L-G-T-M from me.


I appreciate the feeling, although I wish it had been expressed when we
discussed the general approach on Friday... :).

https://codereview.chromium.org/1323463005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Ensure that implicit return undefined is generated. (issue 1308693014 by rmcil...@chromium.org)

[rmcilroy]

An alternative would be to append "Ldar undefined; return" in  
ToBytecodeArray
and add a comment that this needs to be addressed in the soon to start  
flow

control activity.


As long as we have a clear comment either way, in the near term, then  
lgtm.


Added a comment in HasExplicitReturn. Thanks for the reviews!

https://codereview.chromium.org/1308693014/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Clarify comment about Parameter indexing. (issue 1329043002 by mstarzin...@chromium.org)

[rmcilroy]

Ahh yes, it was the variable indexes which had the '-1', that explains my
confusion. Thanks!

lgtm.

https://codereview.chromium.org/1329043002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

On 2015/09/04 16:46:58, Michael Starzinger wrote:

https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc

File src/compiler/bytecode-graph-builder.cc (right):



https://codereview.chromium.org/1291693004/diff/180001/src/compiler/bytecode-graph-builder.cc#newcode46

src/compiler/bytecode-graph-builder.cc:46: const Operator* receiver_op =
common()->Parameter(-1, nullptr);
Something is highly fishy here. Parameter(0) should refer to the receiver.
Essentially if you call "x.f(a,b,c)", then ...



Parameter(0) == x  // Receiver
Parameter(1) == a  // 1st arg
Parameter(2) == b  // 2nd arg
Parameter(3) == c  // 3rd arg
Parameter(4) == f.context  // Context
Parameter(-1) == f // Closure



Anything else would surprise me if it'd work.


Yeah, I think I was wrong here. I was confused by the comment in  
ASTGraphBuilder

which says:
  // Bind all parameter variables. The parameter indices are shifted by 1
  // (receiver is parameter index -1 but environment index 0).
From this comment it sounds like the reciever is -1, but I guess what it is
actually saying is that the reciever is '0' and all other parameters are  
shifted

up by one because of it. Could we fix the comment?


https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

On 2015/09/07 05:14:30, Benedikt Meurer wrote:

Just two very early comments, and one question:
You mentioned that it should be possible to write certain bytecode  
handlers as
native builtins. How about doing that for the call opcode? Because that  
seems

like a lot of machinery in TurboFan to support this very special opcode.


[Answering question first, I'll fix the other issues once this is decided]

A lot of the machinery comes from the fact that I made this a separate
CallVarArgs ircode as we dicussed offline on Friday to keep things  
seperatable.

I could reduce this machinery a lot by just relying on the kHasVarArgs in
CallDescriptor and augmenting the Call instruction to support this. Would  
this

ease your concerns?

What I mentioned originally was being able to write certain (complex)  
bytecode
handlers in JS instead of raw-assembly - this wouldn't apply in this case.  
It
might be possible to do in native builtins, but I think making a completely  
new

bytecode handler code generation path just for this (fairly fundamental)
bytecode would add even more machinery (although admittedly not in TF).

https://codereview.chromium.org/1323463005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [stubs] Unify the various versions of [[Call]] with CallCallableStub. (issue 1311013008 by bmeu...@chromium.org)

[rmcilroy]

It's just a regular code object. You need to provide a Callable for it in
CodeFactory.
Though this one might be a bit special since it takes a variable number of
arguments (which tho should be hidden by your CallVarArgs).


I thought this might be the case, thanks (just wanted to makes sure since it
doesn't look like there are any builtins there yet, all of them seemed to be
code-stubs).


https://codereview.chromium.org/1311013008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [stubs] Unify the various versions of [[Call]] with CallCallableStub. (issue 1311013008 by bmeu...@chromium.org)

[rmcilroy]

Overall looks good, should be something that can be used by the  
Interpreter. I
tried to figure out how to modify my CL to call this instead of the  
JSFunction

code object directly, however I couldn't find a way to call a non-JS builtin
from TF - is this not possible or am I missing something obvious?

https://codereview.chromium.org/1311013008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [stubs] Unify the various versions of [[Call]] with CallCallableStub. (issue 1311013008 by bmeu...@chromium.org)

[rmcilroy]

Just tried this with the interpreter CallJS WIP CL and it works fine. LGTM,
thanks!

https://codereview.chromium.org/1311013008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Start removing deprecated APIs from cctest (issue 1333463002 by joc...@chromium.org)

[rmcilroy]

lgtm with an optional suggestion. Thanks!


https://codereview.chromium.org/1333463002/diff/1/test/cctest/cctest.h
File test/cctest/cctest.h (right):

https://codereview.chromium.org/1333463002/diff/1/test/cctest/cctest.h#newcode452
test/cctest/cctest.h:452: return v8::Local();
nit - could you create a helper function:

static inline v8::Local CompileRun(v8::Local
source, v8::Local context) {
  v8::Local result;
  if (v8::ScriptCompiler::Compile(context, )
   .ToLocalChecked()
   ->Run(context)
   .ToLocal()) {
 return result;
  }
  return v8::Local();
}

and use it here, below, and for CompileRun on l:395 (with
v8::Isolate::GetCurrent()->GetCurrentContext() passed as context)?

https://codereview.chromium.org/1333463002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

I like the iterator :). A few more comments, I'll look in more depth when  
the

tests are there.


https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode35
src/compiler/bytecode-graph-builder.cc:35: //
nit - drop extra "//" here and at end of comment

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode36
src/compiler/bytecode-graph-builder.cc:36: // values_ layout
nit - /s/values_ layout/The layout of values_ is:/

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode45
src/compiler/bytecode-graph-builder.cc:45: // receiver
nit - /s/receiver/Reciever./ (and similar below)

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode71
src/compiler/bytecode-graph-builder.cc:71: // values layout is
[receiver] [parameters] [registers]
nit - remove this comment (it will probably end up getting out of sync
with the same comment in Environment()).

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/bytecode-graph-builder.cc#newcode285
src/compiler/bytecode-graph-builder.cc:285: BuildBinaryOp(node);
I was meaning that the BuildBinaryOp would take js_op, left, right and
build the node, rather than just finishing the node once it's done
(i.e., move the Node* node = NewNode(js_op, left, right) in each
VisitAdd/Sub... into the BuildBinaryOp.

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h
File src/compiler/linkage.h (right):

https://codereview.chromium.org/1291693004/diff/160001/src/compiler/linkage.h#newcode333
src/compiler/linkage.h:333: static const int
kInterpreterReceiverParameter = -1;
No, please don't make this an Interpreter parameter - these are only the
parameters which are passed via the Dispatch TailCalls in the bytecode
handler. It should probably be just below
kJSFunctionCallClosureParamIndex and be called something like
kJSFunctionRecieverParamIndex (since this is not specific to the
interpreter).

Actually, I'm wondering why this isn't
Linkage::kJSFunctionCallClosureParamIndex - this seems to be what
ASTGraphBuilder is using for the same thing. Michi, why didn't you want
this to be Linkage::kJSFunctionCallClosureParamIndex?

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc
File src/interpreter/bytecode-array-iterator.cc (right):

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc#newcode15
src/interpreter/bytecode-array-iterator.cc:15: :
bytecode_array_(bytecode_array), bytecode_offset_(0) {
need to init operands_used_ if DEBUG

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc#newcode31
src/interpreter/bytecode-array-iterator.cc:31: bool
BytecodeArrayIterator::More() const {
More seems a little confusing here, since it implies you could run until
More() and still call current_bytecode() even if More != true (since it
implies that you can't call Next, not that we have gone past the current
value). How about done() (like StackFrameIteratorBase)? (also maybe
Advance() instead of Next() also like StackFrameIteratorBase?)

https://codereview.chromium.org/1291693004/diff/160001/src/interpreter/bytecode-array-iterator.cc#newcode86
src/interpreter/bytecode-array-iterator.cc:86: void
BytecodeArrayIterator::CheckOperandsUsed() const {
I'm wondering how useful this will be (and whether we will end up
needing to iterate over bytecode and not always check all the bytecode
operands). I'm fine with having it here though if you think it would be
useful.

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [Interpreter] Add support for JS calls. (issue 1323463005 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Benedikt Meurer,

Message:
Hi Benedikt. Here is an in-progress CL which adds support for JS calls to  
the

interpreter as well as adding support for a CallVarArgs instruction in TF as
discussed offline today. Currently it assumes the callable is a JSFunction  
and
calls it directly, but I'll try patching in your CallCallableStub CL on  
Monday

and see if that works. Very happy for any suggestions on how this could be
improved, particularly the TurboFan CallVarArgs bits. PTAL, thanks.

Description:
[Interpreter] Add support for JS calls.

Adds support for JS calls to the interpreter. Requires the addition of
CallVarArg instruction in TurboFan to enable calling with a variable
number of arguments.

Adds the CallJS bytecode.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1323463005/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+380, -12 lines):
  M src/compiler/common-operator.h
  M src/compiler/common-operator.cc
  M src/compiler/instruction-selector.h
  M src/compiler/instruction-selector.cc
  M src/compiler/instruction-selector-impl.h
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/compiler/linkage.h
  M src/compiler/opcodes.h
  M src/compiler/raw-machine-assembler.h
  M src/compiler/raw-machine-assembler.cc
  M src/compiler/typer.cc
  M src/compiler/verifier.cc
  M src/compiler/x64/code-generator-x64.cc
  M src/compiler/x64/instruction-codes-x64.h
  M src/compiler/x64/instruction-selector-x64.cc
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecodes.h
  M src/interpreter/bytecodes.cc
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/compiler/interpreter-assembler-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Remove GC metadata of code object before serializing. (issue 1313953008 by o...@chromium.org)

[rmcilroy]

lgtm, thanks!

https://codereview.chromium.org/1313953008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

Looking really good, mostly just nits with one optional suggestion.


https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode38
src/compiler/bytecode-graph-builder.cc:38: // array.
nit - update comment (values are no longer pushed to the back of the
array)

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode48
src/compiler/bytecode-graph-builder.cc:48: // TODO(oth): receiver
I think the receiver is just Parameter(-1), so you could probably push
that as:

 const Operator* op = common()->Parameter(-1, nullptr);
 Node* parameter = builder->graph()->NewNode(op,
builder->graph()->start());
 values()->push_back(parameter);

if you want it here.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode63
src/compiler/bytecode-graph-builder.cc:63: return
the_register.ToParameterIndex(parameters_count());
Does this work - I think the receiver will come back as ParameterIndex
'0' and arg0 will come back as ParameterIndex '1' (so I think you need
to push the receiver into values first before the other parameters).

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.cc#newcode164
src/compiler/bytecode-graph-builder.cc:164: // TODO(oth): review
ast-graph-builder equivalent, ie arguments
nit - /s/ie/i.e.,

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.h#newcode39
src/compiler/bytecode-graph-builder.h:39: // Convert values in
bytecode_array to convenient to handle forms.
nit - I would just replace this with a comment describing bytecode_at

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.h#newcode41
src/compiler/bytecode-graph-builder.h:41: // Get constant from operand
at position |offset| in bytecode array.
nit - could you make the comment clearer that this get's the constant in
the constant pool which is specified by the index operand at position
|offset| in the bytecode array.

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.h#newcode46
src/compiler/bytecode-graph-builder.h:46: interpreter::Register
register_at(int offset) const;
Optional suggestion (fine with doing this in a later CL if you agree it
would be worth while). Could we encapsulate these operations in
something like a BytecodeArrayWrapper (or some better name) class -
i.e.:

class BytecodeArrayWrapper {
 public:
  BytecodeArrayWrapper(BytecodeArrayWrapper);

  void next();

  interpreter::Bytecode current_bytecode();
  interpreter::Register register_at(int operand_index);
  Handle constant_at(int operand_index);
  int8_t smi8_at(int operand_index);

 private:
  size_t offset;
}

That way, next() could advance by Bytecodes::Size(current_bytecode())
and the register_at, constant_at, smi8_at etc. could all DCHECK that the
operands are the expected type - i.e.:
  DCHECK_EQ(interpreter::OperandType::kImm8,
interpreter::Bytecodes::GetOperandType(current_bytecode(),
operand_index));

And then the VisitBytecodeX(int offset) functions could be replaced with
VisitBytecodeX(const ByteCodeArrayWrapper& bytecode);

https://codereview.chromium.org/1291693004/diff/140001/src/compiler/bytecode-graph-builder.h#newcode76
src/compiler/bytecode-graph-builder.h:76: void
FinishBinaryOperation(Node* node);
How about we replace this with "Node* BuildBinaryOp(Node* left, Node*
right, Token::Value op);" like ASTGraphBuilder?". I.e., instead of:
  Node* node = NewNode(js_op, left, right);
  FinishBinaryOperation(node);

just:
  Node* node = BuildBinaryOp(js_op, left, right);

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Move register-operand and parameter-operand conversion routines into (issue 1325983002 by o...@chromium.org)

[rmcilroy]

Still lgtm, thanks!


https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecodes.cc#newcode183
src/interpreter/bytecodes.cc:183: DCHECK_LE(parameter_count,
kMaxParameterIndex);
Yes your right, thanks.

https://codereview.chromium.org/1325983002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Move register-operand and parameter-operand conversion routines into (issue 1325983002 by o...@chromium.org)

[rmcilroy]

Looks much cleaner, thanks. Lgtm once comments are addressed.


https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecodes.cc#newcode173
src/interpreter/bytecodes.cc:173:
nit extra newline

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecodes.cc#newcode182
src/interpreter/bytecodes.cc:182: DCHECK_LE(index, parameter_count);
/s/DCHECK_LE/DCHECK_LT, no?

https://codereview.chromium.org/1325983002/diff/40001/src/interpreter/bytecodes.cc#newcode183
src/interpreter/bytecodes.cc:183: DCHECK_LE(parameter_count,
kMaxParameterIndex);
ditto

https://codereview.chromium.org/1325983002/diff/40001/test/unittests/interpreter/bytecodes-unittest.cc
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1325983002/diff/40001/test/unittests/interpreter/bytecodes-unittest.cc#newcode27
test/unittests/interpreter/bytecodes-unittest.cc:27: std::vector
parameter_counts{7, 13, 99};
Unfortunately I don't think this will work on Mac since it is relying on
c++11 library features. :(

Could you just do something like:
int parameter_counts[] = {7, 13, 99};
size_t count = sizeof parameter_counts / sizeof(int);
for (size_t p = 0; p < count; p++) {...

https://codereview.chromium.org/1325983002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for property load operations. (issue 1309843007 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1309843007/diff/60001/src/interpreter/bytecode-generator.cc
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1309843007/diff/60001/src/interpreter/bytecode-generator.cc#newcode318
src/interpreter/bytecode-generator.cc:318: Visit(expr->obj());
On 2015/09/02 08:16:14, Michael Starzinger wrote:

The JavaScript evaluation order is first to evaluate the object and

then

evaluate the key. That is observable with e.g. ...



(o.something_that_prints())[o.something_that_throws()];


Thanks, I should have paid more attention to this! Done, by changing
round the LoadIC bytecode to expect "name" in the accumulator and
"object" in the named register.

This is actually better overall, since once we can eliminate redundant
register load/store operations the common case (where the object is a
local or a parameter) can turn into:
  LoadConstant 
  LoadIC  

instead of the previous:
  LoadConstant 
  Star 
  Lrda 
  LoadIC  

https://codereview.chromium.org/1309843007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Move register-operand and parameter-operand conversion routines into (issue 1325983002 by o...@chromium.org)

[rmcilroy]

Looks good. My main suggestion is whether we can move Register class to
bytecodes.h to keep all the logic together a bit more.


https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecode-array-builder.h#newcode93
src/interpreter/bytecode-array-builder.h:93: class Register {
Would it be simpler just to move the Register class over to bytecodes.h?
(Genuine question)

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode123
src/interpreter/bytecodes.cc:123: uint8_t
Bytecodes::ParameterIndexToOperand(int index, int parameter_count) {
Could we make this a Register::FromParameterIndex() instead (and then
can use Register::ToOperand if we need the operand)? This is assuming we
can move Register class over to bytecodes.h as in my earlier comment.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode126
src/interpreter/bytecodes.cc:126: DCHECK_GE(index, 0);
nit - DCHECK_LT(index, parameter_count)

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode144
src/interpreter/bytecodes.cc:144: return 128 + kLastParamRegisterIndex;
This is a bit confusing. Could you use kMinRegisterIndex with the -1
adjustment (from count to index) and mention that
kLastParamRegisterIndex is negative.

https://codereview.chromium.org/1325983002/diff/20001/src/interpreter/bytecodes.cc#newcode186
src/interpreter/bytecodes.cc:186: os << "this";
nit - could we make it "" or "|this|" or something to make it
clear this is a parameter? Maybe do the same with "" or "|a0|"as
well?

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc
File test/unittests/interpreter/bytecodes-unittest.cc (right):

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc#newcode24
test/unittests/interpreter/bytecodes-unittest.cc:24: int
parameter_counts[] = {7, 13, 99};
nit - could you just use a std::vector here instead of having to define
COUNT_OF?

https://codereview.chromium.org/1325983002/diff/20001/test/unittests/interpreter/bytecodes-unittest.cc#newcode43
test/unittests/interpreter/bytecodes-unittest.cc:43: for (int i = 0; i <
128; i++) {
nit - /s/128/kMaxRegisterIndex

https://codereview.chromium.org/1325983002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Code::WipeOutHeader needs to null out the next code link to avoid (issue 1310503006 by o...@chromium.org)

[rmcilroy]

lgtm with a question for Hannes.


https://codereview.chromium.org/1310503006/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1310503006/diff/1/src/objects-inl.h#newcode6420
src/objects-inl.h:6420: WRITE_FIELD(this, kNextCodeLinkOffset, NULL);
+hpayer

Should we also wipeout kGCMetadataOffset ?

https://codereview.chromium.org/1310503006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22
src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph);
On 2015/09/01 15:25:57, oth wrote:

On 2015/09/01 14:00:06, rmcilroy wrote:
> Could we just have the one constructor and one CreateGraph and have

the test

> framework do the necessary work to create an appropriate

CompilationInfo

rather
> than having test specific entry points?



Totally agree with the sentiment. Faking a compilation info looked

messy, see

there's a possible example in RawMachineAssemblerTester::Generate() -

is this

what you're thinking of.


Yes, something like RawMachineAssemblerTester::Generate() in a test
would be fine to me. I would rather have slightly messy code in a test
than two entry points if it's possible.

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for property load operations. (issue 1309843007 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Michael Starzinger, mvstanton, oth,

Message:
Adds support for loadICs to the interpreter.

Michi for TF stuff
Michael for TypeFeedbackVector stuff
Orion for bytecode-generator/array-builder stuff

PTAL, thanks!

Description:
[Interpreter] Add support for property load operations.

Adds support for property load operations via Load/KeyedLoad ICs. Adds the
following bytecodes:
 - LoadIC
 - KeyedLoadIC
Also adds support to the interpreter assembler for loading the type feedback
vector from the function on the stack, and calling ICs.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1309843007/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+526, -95 lines):
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/frames.h
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecode-generator.h
  M src/interpreter/bytecode-generator.cc
  M src/interpreter/bytecodes.h
  M src/interpreter/interpreter.h
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-bytecode-generator.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/compiler/interpreter-assembler-unittest.h
  M test/unittests/compiler/interpreter-assembler-unittest.cc
  M test/unittests/interpreter/bytecode-array-builder-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Skeleton bytecode graph builder [NOT FOR COMMIT] (issue 1291693004 by o...@chromium.org)

[rmcilroy]

Looking good to me. I don't have enough knowledge on TF to comment on the  
graph

building questions, but made a couple of readability nits.


https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode21
src/compiler/bytecode-graph-builder.cc:21: // NB Nodes talk slides:
http://shortn/_fmVf0TjCC4
nit - probably remove the internal talk link

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode46
src/compiler/bytecode-graph-builder.cc:46:
values()->push_back(undefined_constant);
nit - comment that this is the accumulator

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode53
src/compiler/bytecode-graph-builder.cc:53:
values()->push_back(values()->at(values_index));
Do we need to keep the old register values around (e.g., here where we
are pushing back the existing Node* for the register to the end of the
vector before replacing it with the new node)? Seems like we should be
OK just dropping them on the floor since we won't need them after this
point.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode66
src/compiler/bytecode-graph-builder.cc:66:
BindRegister(accumulator_pseudo_register(), node);
nit - could we just have a separate Node* accumulator_ in the
Environment rather than a accumulator_pseudo_register which points to a
location in values to make things a bit clearer?

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode117
src/compiler/bytecode-graph-builder.cc:117: int locals_count =
bytecode_array()->frame_size() / kPointerSize;
optional nit - you could add a locals_count() helper on BytecodeArray
object for this if you like.

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode170
src/compiler/bytecode-graph-builder.cc:170: // TODO(oth): write
ast-graph-builder equivalent.
not sure what this TODO means, could you add more detail or remove the
TODO if it's no longer applicable

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.cc#newcode207
src/compiler/bytecode-graph-builder.cc:207: Handle constant =
FixedArray::get(constants, operand_offset);
nit - create a helper for getting a given constant?

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/1291693004/diff/80001/src/compiler/bytecode-graph-builder.h#newcode22
src/compiler/bytecode-graph-builder.h:22: JSGraph* jsgraph);
Could we just have the one constructor and one CreateGraph and have the
test framework do the necessary work to create an appropriate
CompilationInfo rather than having test specific entry points?

https://codereview.chromium.org/1291693004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for property load operations. (issue 1309843007 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1309843007/diff/20001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1309843007/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode303
src/interpreter/bytecode-array-builder.cc:303: bool
BytecodeArrayBuilder::FitsByteOperand(T value) {
On 2015/09/01 14:01:54, oth wrote:

Template seems a bit gratuitous here - it's invoked with value being

size_t or

int. For template, is_integral would clarify things.



FitsInByteOperand? ValueFitsInByteOperand?



Ahh, is_integral is exactly what I wanted, thanks. Changed to
FitsInByteOperand

https://codereview.chromium.org/1309843007/diff/20001/test/cctest/interpreter/test-bytecode-generator.cc
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1309843007/diff/20001/test/cctest/interpreter/test-bytecode-generator.cc#newcode342
test/cctest/interpreter/test-bytecode-generator.cc:342:
ExpectedSnippet snippets[] = {
On 2015/09/01 14:28:29, mvstanton wrote:

Nice test! Helps to explain what is happening...


Acknowledged.

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interpreter-assembler.cc
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interpreter-assembler.cc#newcode239
src/compiler/interpreter-assembler.cc:239: Node*
InterpreterAssembler::CallIC(CallInterfaceDescriptor descriptor,
On 2015/09/01 14:28:29, mvstanton wrote:

If the number of arguments are going to be fixed I'd like to see

something in

the name calling it a LoadIC, since StoreIC and other types of ICs may

have

different numbers of arguments.



Maybe an ic_kind (Code::LOAD_IC, etc.) parameter could be passed?


My plan was to have overloads for the number of parameters once we need
other CallICs with a different number of arguments (similar to
CallJSBuiltin below). I didn't want to make these specific to a
particular IC type, this is really just meant to be a low-level operator
which calls the given descriptor with the given arguments. WDYT?

https://codereview.chromium.org/1309843007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Use ShouldEnsureSpaceForLazyDeopt more. (issue 1310283005 by tit...@chromium.org)

[rmcilroy]

lgtm, thanks!

https://codereview.chromium.org/1310283005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for loading literals from the constant pool. (issue 1321663003 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/27 20:01:29, titzer wrote:

On 2015/08/27 19:55:36, Michael Starzinger wrote:



https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc

 File test/cctest/interpreter/test-bytecode-generator.cc (right):




https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc#newcode257

 test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
 On 2015/08/27 19:36:29, Michael Starzinger wrote:
  On 2015/08/27 16:34:07, rmcilroy wrote:
   Michi - looks like double literals aren't de-duped by the parser  
(we get

a

 new
   HeapNumber each time). Is this done elsewhere or have we always had  
a

new

   HeapNumber for each double literal?
 
  Not sure I understand your comment correctly. How would the parser  
de-dupe

  literals? That would make the AST degenerate into a DAG if we would do

that.

 Or
  am I missing something here?
 
  Compilers can de-dupe the values. One mechanism of de-duping would be  
GVN

 which
  makes sure that identical nodes in the graph are coalesced. Both  
Hydrogen

and
  TurboFan do that.

 Ah, I think now I understand where you are coming from, I think your

question
 was whether AstValue::Internalize should canonicalize HeapNumbers or  
not.

Maeh,
 the data-structure for canonicalizing them would probably end up eating  
more
 memory than the duped HeapNumbers, I don't think that would buy us  
anything

 besides complexity.

 Rather the question is, why do we internalize double literals in the  
first
 place. One could argue that it should be the responsibility of the  
compiler

to
 decide whether and where to allocate HeapNumbers. But that's opening a  
can

of

 worms. I suggest we don't go there.


The JSGraph in TurboFan de-dupes all constants (including HeapNumbers),  
except
other HeapObjects. I agree with Michi that the parser shouldn't try to  
de-dup

doubles; it's probably not worth it.


Right, sorry I wasn't so clear here. I meant canonicalizing doubles, not  
GVN or
anything like that. I wasn't suggesting we change the parser to do this, I  
just
wanted to make sure I wasn't missing something (e.g., Strings are  
canonicalized,

just wanted to make sure I wasn't missing something similar for double
literals).

If we were going to this I agree it should be the responsibility of the
compliler / bytecode-array-builder (only canonicaling on a per-function  
level),
but it seems from your comments that this is unlikely to be much of a  
benefit,
so I'm quite happy to leave it as a comment for now and possibly  
investigate it

later once we can run real JS.

https://codereview.chromium.org/1321663003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [heap] Move IdentityMap data structure out of heap. (issue 1320503004 by mstarzin...@chromium.org)

[rmcilroy]

lgtm, thanks!

https://codereview.chromium.org/1320503004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for loading literals from the constant pool. (issue 1321663003 by rmcil...@chromium.org)

[rmcilroy]

Review comments addressed, PTAL, thanks.


https://codereview.chromium.org/1321663003/diff/40001/src/DEPS
File src/DEPS (right):

https://codereview.chromium.org/1321663003/diff/40001/src/DEPS#newcode8
src/DEPS:8: +src/heap/identity-map.h,
Done (thanks for doing the move!)

https://codereview.chromium.org/1321663003/diff/40001/src/heap/identity-map.h
File src/heap/identity-map.h (right):

https://codereview.chromium.org/1321663003/diff/40001/src/heap/identity-map.h#newcode9
src/heap/identity-map.h:9: // Do not include anything from src/heap
here!
On 2015/08/28 12:21:04, Michael Starzinger wrote:

Likewise.


Done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecode-array-builder.cc#newcode55
src/interpreter/bytecode-array-builder.cc:55: if (constants_.size() ==
0) {
On 2015/08/28 12:21:04, Michael Starzinger wrote:

This special-casing is already part of Factory::NewFixedArray, it

should fall

out naturally of the code below.


Perfect! thanks, done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecode-array-builder.h#newcode27
src/interpreter/bytecode-array-builder.h:27: explicit
BytecodeArrayBuilder(Isolate* isolate, Zone* zone);
On 2015/08/28 12:21:04, Michael Starzinger wrote:

nit: No longer needs to be marked as explicit.


Done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecode-array-builder.h#newcode79
src/interpreter/bytecode-array-builder.h:79: std::vectoruint8_t
bytecodes_;
On 2015/08/28 12:21:04, Michael Starzinger wrote:

Same comment from below applies here.


Done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecode-array-builder.h#newcode83
src/interpreter/bytecode-array-builder.h:83: std::vectorHandleObject
constants_;
On 2015/08/28 12:21:04, Michael Starzinger wrote:

Could we use a ZoneVector here, that would provide us with accounting

for the

(zone) memory footprint of the interpreter later (i.e. similar to

--turbo-stats)

Done.

https://codereview.chromium.org/1321663003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for loading literals from the constant pool. (issue 1321663003 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Michael Starzinger,

Message:
Michi, could you take a look please? Thanks.


https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter/test-bytecode-generator.cc#newcode257
test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
Michi - looks like double literals aren't de-duped by the parser (we get
a new HeapNumber each time). Is this done elsewhere or have we always
had a new HeapNumber for each double literal?

Description:
[Interpreter] Add support for loading literals from the constant pool.

Adds support to the interpreter for loading literals from the constant pool.
Adds the LoadConstant bytecode and makes use of it for loading large Smis  
and

HeapObject literals.

Also removes unused HandleVector from utils.h.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1321663003/

Base URL: https://chromium.googlesource.com/v8/v8.git@int_const_pool_1

Affected files (+407, -75 lines):
  M src/DEPS
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/heap/identity-map.h
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecode-generator.cc
  M src/interpreter/bytecodes.h
  M src/interpreter/bytecodes.cc
  M src/interpreter/interpreter.cc
  M src/utils.h
  M test/cctest/interpreter/test-bytecode-generator.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/compiler/interpreter-assembler-unittest.cc
  M test/unittests/interpreter/bytecode-array-builder-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for parameter variables. (issue 1303403004 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1303403004/diff/140001/src/arm64/builtins-arm64.cc
File src/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/1303403004/diff/140001/src/arm64/builtins-arm64.cc#newcode1032
src/arm64/builtins-arm64.cc:1032: __ Drop(x1);
On 2015/08/26 13:44:36, Michael Starzinger wrote:

Not sure but isn't the ARM64 macro assembler expecting count instead

of size

here when a register is passed in? Shouldn't this be Drop(x1, 1)

instead?

Yes, good catch thanks!

https://codereview.chromium.org/1303403004/diff/140001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1303403004/diff/140001/src/frames.h#newcode183
src/frames.h:183:
On 2015/08/26 13:44:36, Michael Starzinger wrote:

nit: Two empty newlines.


Done.

https://codereview.chromium.org/1303403004/diff/140001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1303403004/diff/140001/src/objects-inl.h#newcode4063
src/objects-inl.h:4063: return READ_INT_FIELD(this,
kParameterSizeOffset)  kPointerSizeLog2;
On 2015/08/26 13:44:36, Michael Starzinger wrote:

Can we leave a short one-liner comment here that the count is stored

as a

size so that generated code can use it directly?


Done.


Also is this trick really worth it? Don't most of the architectures

should

have instructions with the appropriate scalling factors? Not an expert

on this,

so I don't mind either way.


I don't think they are always available AFAIKT. E.g., on Arm you can
scale and immediate add or scale the offset for an ldr, but you can't
scale a reg+reg add or the result of an ldr which is what would be
needed here.

https://codereview.chromium.org/1303403004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Add constant_pool() to BytecodeArray. (issue 1314953004 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1314953004/diff/20001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1314953004/diff/20001/test/cctest/test-heap.cc#newcode710
test/cctest/test-heap.cc:710: i::FLAG_always_compact = true;
On 2015/08/26 22:19:49, Hannes Payer wrote:

The flag you wanna set here is:

FLAG_manual_evacuation_candidates_selection =

true;



Please set the flags in the beginning of the test, before initializing

the VM.

Done.

https://codereview.chromium.org/1314953004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Move (uppercase) JS builtins from js builtins object to native context. (issue 1316943002 by yang...@chromium.org)

[rmcilroy]

Interpreter LGTM with one nit.


https://codereview.chromium.org/1316943002/diff/1/test/unittests/compiler/interpreter-assembler-unittest.cc
File test/unittests/compiler/interpreter-assembler-unittest.cc (right):

https://codereview.chromium.org/1316943002/diff/1/test/unittests/compiler/interpreter-assembler-unittest.cc#newcode276
test/unittests/compiler/interpreter-assembler-unittest.cc:276:
IsIntPtrConstant(Context::SlotOffset(22;
nit - could you add a test for the new m.LoadContextSlot(Node* context,
int slot_index) method you added here please.

https://codereview.chromium.org/1316943002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Make FlushICache NOP for Nvidia Denver 1.0 only (issue 1287173004 by sbo...@nvidia.com)

[rmcilroy]

+hablich for merge request (see comments above).

https://codereview.chromium.org/1287173004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Move (uppercase) JS builtins from js builtins object to native context. (issue 1316943002 by yang...@chromium.org)

[rmcilroy]

Interpreter changes look good to me.

https://codereview.chromium.org/1316943002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Add constant_pool() to BytecodeArray. (issue 1314953004 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Hannes Payer,

Message:
Hannes, could you take a look please? Please let me know if I've missed any
important visitors with this change.

Michi for FYI since I plan to send you the followup CL (but comments  
welcome).


Description:
[interpreter] Add constant_pool() to BytecodeArray.

Adds a (currently unused) constant_pool() field to BytecodeArray objects.
This field points to a FixedArray object which will be used to hold  
constants.


The BytecodeArray is now a mixed values object type, with the
kConstantPoolOffset object holding a tagged pointer, but the remainder of  
the
object holding raw bytes (which could look like tagged pointers but are  
not).

Modify the BytecodeArray GC visitors to deal with this and test that the
field is migrated properly when evacuated.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1314953004/

Base URL: https://chromium.googlesource.com/v8/v8.git@int_args

Affected files (+102, -24 lines):
  M src/factory.h
  M src/factory.cc
  M src/heap/heap.h
  M src/heap/heap.cc
  M src/heap/mark-compact.cc
  M src/heap/objects-visiting.h
  M src/heap/objects-visiting.cc
  M src/heap/objects-visiting-inl.h
  M src/heap/store-buffer.cc
  M src/interpreter/bytecode-array-builder.cc
  M src/objects.h
  M src/objects.cc
  M src/objects-debug.cc
  M src/objects-inl.h
  M test/cctest/test-heap.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add support for parameter variables. (issue 1303403004 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Michael Starzinger,

Message:
Michael, could you take a look please, thanks.

Description:
[Interpreter] Add support for parameter variables.

Adds support for parameters to the BytecodeArrayBuilder and  
BytecodeGenerator.

Parameters are accessed as negative interpreter registers.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1303403004/

Base URL: https://chromium.googlesource.com/v8/v8.git@int_add_bytecodes

Affected files (+317, -86 lines):
  M src/arm/builtins-arm.cc
  M src/arm64/builtins-arm64.cc
  M src/factory.h
  M src/factory.cc
  M src/frames.h
  M src/heap/heap.h
  M src/heap/heap.cc
  M src/ia32/builtins-ia32.cc
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecode-generator.cc
  M src/mips/builtins-mips.cc
  M src/mips64/builtins-mips64.cc
  M src/objects.h
  M src/objects.cc
  M src/objects-inl.h
  M src/x64/builtins-x64.cc
  M test/cctest/interpreter/test-bytecode-generator.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/cctest/test-heap.cc
  M test/unittests/interpreter/bytecode-array-builder-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Allow verification and trace-turbo for bytecode handlers. (issue 1308863004 by rmcil...@chromium.org)

[rmcilroy]

Ping?

https://codereview.chromium.org/1308863004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot (issue 1294133004 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1294133004/diff/20001/src/compiler/linkage.cc
File src/compiler/linkage.cc (right):

https://codereview.chromium.org/1294133004/diff/20001/src/compiler/linkage.cc#newcode416
src/compiler/linkage.cc:416: #if defined(V8_TARGET_ARCH_IA32)
On 2015/08/20 09:32:02, Michael Starzinger wrote:

Just for posterity: Not a huge fan of this #ifdef here, but let's keep

it like

it is and not overly abstract is for now, because it might change in

the future

anyways.


Acknowledged.

https://codereview.chromium.org/1294133004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot (issue 1294133004 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/20 09:32:02, Michael Starzinger wrote:
LGTM. As discussed offline: I am fine with landing this as it is to  
unblock

interpreter work. We should just keep the alternative ideas about context

chain
access in mind. This might mean that we'd potentially remove this  
parameter

again if we decide to take an alternative approach.


As discussed offline, the current plan is to do context renaming to put  
context
chain extensions into registers, but also maintain a current context in the  
cp

register (or a stack slot on ia32) to avoid the need for an extra context
operand on operations which act on the current context.

I wrote some more details in the design doc at:
https://docs.google.com/document/d/11T2CRex9hXxoJwbYqVQ32yIPMh0uouUZLdyrtmMoL44/edit?pli=1#heading=h.wad4no4ny54w

https://codereview.chromium.org/1294133004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add implementations of arithmetic binary op bytecodes. (issue 1300813005 by rmcil...@chromium.org)

[rmcilroy]

Comments addressed, PTAL.


https://codereview.chromium.org/1300813005/diff/1/src/compiler/interpreter-assembler.cc
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1300813005/diff/1/src/compiler/interpreter-assembler.cc#newcode225
src/compiler/interpreter-assembler.cc:225: return CallJSBuiltin(builtin,
receiver, args, 1);
On 2015/08/19 17:18:56, Michael Starzinger wrote:

nit: Shouldn't the following do the trick ...



return CallJSBuiltin(builtin, receiver, arg1, 1);


Yup :). Done.

https://codereview.chromium.org/1300813005/diff/1/src/compiler/raw-machine-assembler.cc
File src/compiler/raw-machine-assembler.cc (right):

https://codereview.chromium.org/1300813005/diff/1/src/compiler/raw-machine-assembler.cc#newcode146
src/compiler/raw-machine-assembler.cc:146: buffer[index++] =
graph()-start();
On 2015/08/19 17:18:56, Michael Starzinger wrote:

Please don't add effect and control inputs to the call in the raw

machine

assembler.


As discussed in https://codereview.chromium.org/1283193007/, there are
various reasons why we want to do this.

https://codereview.chromium.org/1300813005/diff/1/src/compiler/raw-machine-assembler.cc#newcode148
src/compiler/raw-machine-assembler.cc:148: CallDescriptor* descriptor =
Linkage::GetJSCallDescriptor(
On 2015/08/19 17:18:56, Michael Starzinger wrote:

Would it be possible to just use RawMachineAssembler::CallN instead

and move

computation of the call-descriptor to the caller (i.e. into
InterpreterAssembler) instead?


Done (also removed CallJs0 and renamed CallInterpreterDispatch -
TailCallN.

https://codereview.chromium.org/1300813005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Add control and effect inputs to RawMachineAssembler calls. (issue 1283193007 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/21 10:31:52, titzer wrote:

*sigh*



I'll be ok with this patch, but keep in mind that the graphs built by raw
machine assembler are really broken and don't make sense at all without a
schedule.


Noted, thanks. PTAL.


https://codereview.chromium.org/1283193007/diff/20001/src/compiler/graph.h
File src/compiler/graph.h (right):

https://codereview.chromium.org/1283193007/diff/20001/src/compiler/graph.h#newcode82
src/compiler/graph.h:82: Node* NewNode(const Operator* op, Node* n1,
Node* n2, Node* n3, Node* n4,
On 2015/08/21 10:31:52, titzer wrote:

We've got too many overloads here already, can we use the generic

method in the

callers instead?


Done.

https://codereview.chromium.org/1283193007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Allow verification and trace-turbo for bytecode handlers. (issue 1297203002 by rmcil...@chromium.org)

[rmcilroy]

Updated to replace !info-IsStub()  !info-IsBytecodeHandler() with a  
new
ShouldEnsureSpaceForLazyDeopt() function based on offline discussion with  
Danno.



Ben, could you PTAL? Thanks.

https://codereview.chromium.org/1297203002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Allow verification and trace-turbo for bytecode handlers. (issue 1297203002 by rmcil...@chromium.org)

[rmcilroy]

+Yang for serializer/ review.

Ben: I've reworked things to have a GetDebugName on CompilerInfo. I've  
replaced
all the copy/pasted code which got the debug name from the CompilerInfo  
with a
call to this new function and have done a couple of other cleanups  
(removing a

useless parameter on MajorName() and removing FakeStubForTesting which is no
longer required).

I've taken out all of the changes required for interpreter verification from
this CL (I'll land that as a followup) and this CL should now only be a  
refactor

CL with no functional changes. PTAL, thanks.


https://codereview.chromium.org/1297203002/diff/60001/src/codegen.cc
File src/codegen.cc (right):

https://codereview.chromium.org/1297203002/diff/60001/src/codegen.cc#newcode160
src/codegen.cc:160: info-IsStub() || info-IsBytecodeHandler();
On 2015/08/21 13:37:21, titzer wrote:

Here too


Removed this (it's not about lazy deopt, so didn't replace it with
ShouldEnsureSpaceForLazyDeopt()).

https://codereview.chromium.org/1297203002/diff/60001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1297203002/diff/60001/src/compiler.h#newcode441
src/compiler.h:441: BYTECODE_HANDLER
On 2015/08/21 13:37:21, titzer wrote:

Do we really need a new mode? What does it control?


Removed this mode and replaced it's need with debug_name.

https://codereview.chromium.org/1297203002/diff/60001/src/compiler.h#newcode467
src/compiler.h:467: interpreter::Bytecode bytecode_;
On 2015/08/21 13:37:21, titzer wrote:

AFAICT this is only used for debugging purposes in printing to the log

file. Can

we find another way to handle that without adding more stuff to the

(already

bloated) compilation info?


Done with GetDebugName as discussed.

https://codereview.chromium.org/1297203002/diff/60001/src/compiler/code-generator.cc
File src/compiler/code-generator.cc (right):

https://codereview.chromium.org/1297203002/diff/60001/src/compiler/code-generator.cc#newcode196
src/compiler/code-generator.cc:196: if (!info-IsStub() 
!info-IsBytecodeHandler()) {
On 2015/08/21 13:37:21, titzer wrote:

Missed this one.


Done.

https://codereview.chromium.org/1297203002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [interpreter] Allow verification and trace-turbo for bytecode handlers. (issue 1308863004 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: titzer,

Message:
This is the follow-on from https://codereview.chromium.org/1297203002/.  
PTAL,

thanks.

Description:
[interpreter] Allow verification and trace-turbo for bytecode handlers.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1308863004/

Base URL: https://chromium.googlesource.com/v8/v8.git@ignition-visualize

Affected files (+43, -4 lines):
  M src/compiler/interpreter-assembler.cc
  M src/compiler/pipeline.h
  M src/compiler/pipeline.cc
  M src/compiler/raw-machine-assembler.cc


Index: src/compiler/interpreter-assembler.cc
diff --git a/src/compiler/interpreter-assembler.cc  
b/src/compiler/interpreter-assembler.cc
index  
47e014ba3992b3aa2e1b1ce4b191b0f76b15930c..8c2de42aa5297e7481c64f3a7ffbd89eedc96324  
100644

--- a/src/compiler/interpreter-assembler.cc
+++ b/src/compiler/interpreter-assembler.cc
@@ -44,15 +44,17 @@ HandleCode InterpreterAssembler::GenerateCode() {

   End();

+  const char* bytecode_name = interpreter::Bytecodes::ToString(bytecode_);
   Schedule* schedule = raw_assembler_-Export();
   // TODO(rmcilroy): use a non-testing code generator.
-  HandleCode code = Pipeline::GenerateCodeForTesting(
-  isolate(), raw_assembler_-call_descriptor(), graph(), schedule);
+  HandleCode code = Pipeline::GenerateCodeForInterpreter(
+  isolate(), raw_assembler_-call_descriptor(), graph(), schedule,
+  bytecode_name);

 #ifdef ENABLE_DISASSEMBLER
   if (FLAG_trace_ignition_codegen) {
 OFStream os(stdout);
-code-Disassemble(interpreter::Bytecodes::ToString(bytecode_), os);
+code-Disassemble(bytecode_name, os);
 os  std::flush;
   }
 #endif
Index: src/compiler/pipeline.cc
diff --git a/src/compiler/pipeline.cc b/src/compiler/pipeline.cc
index  
b627f25ed6259aecd7ab53a52c7be2fb9197fee0..ffeec468c07285cc36d25be8457901854287c201  
100644

--- a/src/compiler/pipeline.cc
+++ b/src/compiler/pipeline.cc
@@ -1129,6 +1129,36 @@ HandleCode Pipeline::GenerateCode() {
 }


+HandleCode Pipeline::GenerateCodeForInterpreter(
+Isolate* isolate, CallDescriptor* call_descriptor, Graph* graph,
+Schedule* schedule, const char* bytecode_name) {
+  CompilationInfo info(bytecode_name, isolate, graph-zone());
+
+  // Construct a pipeline for scheduling and code generation.
+  ZonePool zone_pool;
+  PipelineData data(zone_pool, info, graph, schedule);
+  base::SmartPointerPipelineStatistics pipeline_statistics;
+  if (FLAG_turbo_stats) {
+pipeline_statistics.Reset(new PipelineStatistics(info, zone_pool));
+pipeline_statistics-BeginPhaseKind(interpreter handler codegen);
+  }
+  if (FLAG_trace_turbo) {
+FILE* json_file = OpenVisualizerLogFile(info, NULL, json, w+);
+if (json_file != nullptr) {
+  OFStream json_of(json_file);
+  json_of  {\function\:\  info.GetDebugName().get()
+   \, \source\:\\,\n\phases\:[;
+  fclose(json_file);
+}
+  }
+
+  Pipeline pipeline(info);
+  pipeline.data_ = data;
+  pipeline.RunPrintAndVerify(Machine, true);
+  return pipeline.ScheduleAndGenerateCode(call_descriptor);
+}
+
+
 HandleCode Pipeline::GenerateCodeForTesting(CompilationInfo* info,
   Graph* graph,
   Schedule* schedule) {
Index: src/compiler/pipeline.h
diff --git a/src/compiler/pipeline.h b/src/compiler/pipeline.h
index  
ea8b7e9b4bd0ccc5b4467c3da094b23e71b856e2..4e3635d0f69c5b98009b1f3063c4a61c421a1958  
100644

--- a/src/compiler/pipeline.h
+++ b/src/compiler/pipeline.h
@@ -28,6 +28,12 @@ class Pipeline {
   // Run the entire pipeline and generate a handle to a code object.
   HandleCode GenerateCode();

+  // Run the pipeline on an interpreter bytecode handler machine graph and
+  // generate code.
+  static HandleCode GenerateCodeForInterpreter(
+  Isolate* isolate, CallDescriptor* call_descriptor, Graph* graph,
+  Schedule* schedule, const char* bytecode_name);
+
   // Run the pipeline on a machine graph and generate code. If {schedule}  
is

   // {nullptr}, then compute a new schedule for code generation.
   static HandleCode GenerateCodeForTesting(CompilationInfo* info,
Index: src/compiler/raw-machine-assembler.cc
diff --git a/src/compiler/raw-machine-assembler.cc  
b/src/compiler/raw-machine-assembler.cc
index  
f437922fa6aa3363b1b908d2ada508624c5ae002..71c79eb9da6e768243a3a1f9f453411edb3846a0  
100644

--- a/src/compiler/raw-machine-assembler.cc
+++ b/src/compiler/raw-machine-assembler.cc
@@ -25,7 +25,8 @@ RawMachineAssembler::RawMachineAssembler(Isolate*  
isolate, Graph* graph,

   parameters_(nullptr),
   current_block_(schedule()-start()) {
   int param_count = static_castint(parameter_count());
-  Node* s = graph-NewNode(common_.Start(param_count));
+  // Add an extra input node for the JSFunction parameter to the start  
node.

+  Node* s = graph-NewNode(common_.Start(param_count + 1));
   graph-SetStart(s);
   if (parameter_count() == 0) return;
   parameters_ = zone()-NewArrayNode

[v8-dev] Re: [Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot (issue 1294133004 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/19 16:41:52, Michael Starzinger wrote:

On 2015/08/19 15:42:16, rmcilroy wrote:
 On 2015/08/19 15:15:37, Michael Starzinger wrote:
  High level question: Do we need to keep the context in a machine  
register?

Or
  could be but it into one of the interpreter registers thereby avoiding
pinning
  yet another machine register.

 I don't want to have it in an interpreter Register, otherwise the
 BytecodeArrayBuilder needs to know about it, which could get messy.

 On ia32 it is already passed as a stack slot (effectively a stack  
located
 argument to the Bytecode handler) so it isn't using an extra register  
there.

For
 other architectures we have plenty of registers and I would prefer we  
have

it

in
 the standard context register, that way we don't need to load it every  
time

we

 call another JS function.


I would be fine with going with such an implementation. But the  
higher-level
issue I wanted to raise is that it's not entirely clear to me how the  
context
chain should be represented in the bytecode. Going with this approach I  
assume
means that there is one dedicated context parameter to each bytecode  
handler
which most handlers will pass through only some {ContextPush} and  
{ContextPop}
will actually update that parameter to point to something else before  
doing

the

next interpreter dispatch. That in turn means that the translation from

bytecode

to a graph would then SSA-rename this slot when compiling from bytecode.

Totally

valid approach.



An alternative approach would be, to do the renaming when building the

bytecode,

put the context into an interpreter register, also put context chain

extensions

into (other) interpreter registers. This would make {ContextPush} and
{ContextPop} bytecodes unnecessary and the translation from bytecode to a

graph
would also look different. Not saying this approach is better, just  
wanted to

throw it out there.


Ahh right, I see where your coming from now, thanks. I hadn't thought of the
approach of doing SSA renaming during bytecode generation for the context  
chain
(I was planning on doing the ContextPush/Pop approach you mention  
initially).
I'll have think about this approach and what it's implications might be for  
both
the bytecode, the bytecode generator and interpreter. We should probably  
sync up
offline to chat about this - I'll schedule something if we don't get to it  
in

the sub team meeting.

https://codereview.chromium.org/1294133004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Turn v8.h into a normal header. (issue 1293593005 by mstarzin...@chromium.org)

[rmcilroy]

On 2015/08/20 05:34:55, Benedikt Meurer wrote:

LGTM!!


Lgtm

https://codereview.chromium.org/1293593005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot (issue 1294133004 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Michael Starzinger, oth,

Message:
Michael / Orion, PTAL, thanks. This is required for calling the  
Add/Sub/Div/Mod
JS builtins for the respective bytecode implementations (the next CL I'll  
send

out).

Description:
[Interpreter] Pass context to interpreter bytecode handlers and add
LoadConstextSlot

Passes the current context to bytecode interpreter handlers. This is held  
in the
context register on all architectures except for ia32 where there are too  
few

registers and it is instead spilled to the stack.

Also changes Load/StoreRegister to use kMachAnyTagged representation since  
they

should only ever hold tagged values.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1294133004/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+133, -16 lines):
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/compiler/linkage.h
  M src/compiler/linkage.cc
  M src/compiler/raw-machine-assembler.h
  M src/compiler/raw-machine-assembler.cc
  M src/ia32/builtins-ia32.cc
  M src/ia32/macro-assembler-ia32.h
  M test/unittests/compiler/interpreter-assembler-unittest.cc
  M test/unittests/compiler/node-test-utils.h
  M test/unittests/compiler/node-test-utils.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add implementations of arithmetic binary op bytecodes. (issue 1300813005 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Michael Starzinger, oth,

Message:
Michael / Orion, could you please take a look, thanks.

Description:
[Interpreter] Add implementations of arithmetic binary op bytecodes.

Adds implementations and tests for the following bytecodes:
  - Add
  - Sub
  - Mul
  - Div
  - Mod

Also adds the Mod bytecode and adds support to BytecodeGenerator and
BytecodeArrayBuilder to enable it's use.

The current bytecodes always call through to the JS builtins. This also adds
LoadObjectField and CallJSBuiltin operators to the InterpreterAssembler.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1300813005/

Base URL: https://chromium.googlesource.com/v8/v8.git@mstar_v8h

Affected files (+310, -17 lines):
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/compiler/raw-machine-assembler.h
  M src/compiler/raw-machine-assembler.cc
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecodes.h
  M src/interpreter/interpreter.h
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/compiler/instruction-selector-unittest.cc
  M test/unittests/compiler/interpreter-assembler-unittest.cc
  M test/unittests/compiler/node-test-utils.h
  M test/unittests/compiler/node-test-utils.cc
  M test/unittests/interpreter/bytecode-array-builder-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Pass context to interpreter bytecode handlers and add LoadConstextSlot (issue 1294133004 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/19 15:15:37, Michael Starzinger wrote:
High level question: Do we need to keep the context in a machine  
register? Or
could be but it into one of the interpreter registers thereby avoiding  
pinning

yet another machine register.


I don't want to have it in an interpreter Register, otherwise the
BytecodeArrayBuilder needs to know about it, which could get messy.

On ia32 it is already passed as a stack slot (effectively a stack located
argument to the Bytecode handler) so it isn't using an extra register  
there. For
other architectures we have plenty of registers and I would prefer we have  
it in
the standard context register, that way we don't need to load it every time  
we

call another JS function.

https://codereview.chromium.org/1294133004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Add control and effect inputs to RawMachineAssembler calls. (issue 1283193007 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/19 15:16:17, titzer wrote:

On 2015/08/19 06:55:32, rmcilroy wrote:
 Ben, please take a look, thanks.


What's the motivation for this? The graphs built by the raw assembler  
already
have no hope of being verified because they don't have proper effect  
chains

nor

control chains.


There are a couple of motivations
 - The bytecode handlers can get through verification with the changes in
https://codereview.chromium.org/1297203002/, but they wouldn't if I call  
any of
these functions without this change (note, the callInterpreterDispatch  
already

has effect/control edges due to the point below)
 - The IsCall node matchers used by interpreter-assembler-unittests expect
control/effect edges and fail if they aren't passed.
 - The Load/Store operations already hook up their control/effect edges to
graph-start().
 - I've ended up with bugs while implementing where I passed x value inputs  
but
the call descriptor only expected x-1 and it was difficult to work out why  
the
wrong code was being generated until I figured out one of the value inputs  
was

being treated as the control edge.

https://codereview.chromium.org/1283193007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Add control and effect inputs to RawMachineAssembler calls. (issue 1283193007 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: titzer,

Message:
Ben, please take a look, thanks.

Description:
[turbofan] Add control and effect inputs to RawMachineAssembler calls.

Calls should have control and effect inputs, which were previously missing
for RawMachineAssembler call operations. Add control and effect edges to
graph()-start().

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1283193007/

Base URL:  
ssh://rmcilroy.lon.corp.google.com///usr/local/google/code/v8_full/v8@master


Affected files (+30, -12 lines):
  M src/compiler/graph.h
  M src/compiler/raw-machine-assembler.cc


Index: src/compiler/graph.h
diff --git a/src/compiler/graph.h b/src/compiler/graph.h
index  
cb073b312acafdc01933bbb6f113be18a714f1c4..aa1977084bebb59f86cced1929520efd3a826b1c  
100644

--- a/src/compiler/graph.h
+++ b/src/compiler/graph.h
@@ -79,6 +79,17 @@ class Graph : public ZoneObject {
 Node* nodes[] = {n1, n2, n3, n4, n5, n6, n7, n8, n9};
 return NewNode(op, arraysize(nodes), nodes);
   }
+  Node* NewNode(const Operator* op, Node* n1, Node* n2, Node* n3, Node* n4,
+Node* n5, Node* n6, Node* n7, Node* n8, Node* n9, Node*  
n10) {

+Node* nodes[] = {n1, n2, n3, n4, n5, n6, n7, n8, n9, n10};
+return NewNode(op, arraysize(nodes), nodes);
+  }
+  Node* NewNode(const Operator* op, Node* n1, Node* n2, Node* n3, Node* n4,
+Node* n5, Node* n6, Node* n7, Node* n8, Node* n9, Node*  
n10,

+Node* n11) {
+Node* nodes[] = {n1, n2, n3, n4, n5, n6, n7, n8, n9, n10, n11};
+return NewNode(op, arraysize(nodes), nodes);
+  }

   // Clone the {node}, and assign a new node id to the copy.
   Node* CloneNode(const Node* node);
Index: src/compiler/raw-machine-assembler.cc
diff --git a/src/compiler/raw-machine-assembler.cc  
b/src/compiler/raw-machine-assembler.cc
index  
8013f422f6b9ba42bd825b068367f3bc96730e44..15d7b1b1f9e27b86e6d077ad187245135de5cffb  
100644

--- a/src/compiler/raw-machine-assembler.cc
+++ b/src/compiler/raw-machine-assembler.cc
@@ -104,13 +104,15 @@ Node* RawMachineAssembler::CallN(CallDescriptor*  
desc, Node* function,

  Node** args) {
   int param_count =
   static_castint(desc-GetMachineSignature()-parameter_count());
-  Node** buffer = zone()-NewArrayNode*(param_count + 1);
+  Node** buffer = zone()-NewArrayNode*(param_count + 3);
   int index = 0;
   buffer[index++] = function;
   for (int i = 0; i  param_count; i++) {
 buffer[index++] = args[i];
   }
-  Node* call = graph()-NewNode(common()-Call(desc), param_count + 1,  
buffer);

+  buffer[index++] = graph()-start();
+  buffer[index++] = graph()-start();
+  Node* call = graph()-NewNode(common()-Call(desc), param_count + 3,  
buffer);

   schedule()-AddNode(CurrentBlock(), call);
   return call;
 }
@@ -125,7 +127,8 @@ Node* RawMachineAssembler::CallFunctionStub0(Node*  
function, Node* receiver,

   CallDescriptor::kNeedsFrameState, Operator::kNoProperties);
   Node* stub_code = HeapConstant(callable.code());
   Node* call = graph()-NewNode(common()-Call(desc), stub_code, function,
-receiver, context, frame_state);
+receiver, context, frame_state,
+graph()-start(), graph()-start());
   schedule()-AddNode(CurrentBlock(), call);
   return call;
 }
@@ -135,8 +138,9 @@ Node* RawMachineAssembler::CallJS0(Node* function,  
Node* receiver,

Node* context, Node* frame_state) {
   CallDescriptor* descriptor = Linkage::GetJSCallDescriptor(
   zone(), false, 1, CallDescriptor::kNeedsFrameState);
-  Node* call = graph()-NewNode(common()-Call(descriptor), function,  
receiver,

-context, frame_state);
+  Node* call =
+  graph()-NewNode(common()-Call(descriptor), function, receiver,  
context,

+   frame_state, graph()-start(), graph()-start());
   schedule()-AddNode(CurrentBlock(), call);
   return call;
 }
@@ -154,7 +158,8 @@ Node*  
RawMachineAssembler::CallRuntime1(Runtime::FunctionId function,

   Node* arity = Int32Constant(1);

   Node* call = graph()-NewNode(common()-Call(descriptor), centry, arg0,  
ref,

-arity, context, frame_state);
+arity, context, frame_state,  
graph()-start(),

+graph()-start());
   schedule()-AddNode(CurrentBlock(), call);
   return call;
 }
@@ -167,7 +172,8 @@ Node* RawMachineAssembler::CallCFunction0(MachineType  
return_type,

   const CallDescriptor* descriptor =
   Linkage::GetSimplifiedCDescriptor(zone(), builder.Build());

-  Node* call = graph()-NewNode(common()-Call(descriptor), function);
+  Node* call = graph()-NewNode(common()-Call(descriptor), function,
+graph()-start(), graph()-start());
   schedule()-AddNode(CurrentBlock(), call);
   return call;
 }
@@ -182,7 +188,8 @@ Node* 

[v8-dev] Re: [Interpreter] Minimal bytecode generator. (issue 1294543002 by o...@chromium.org)

[rmcilroy]

lgtm with a couple of nits.


https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode23
test/cctest/interpreter/test-bytecode-generator.cc:23:
i::FLAG_print_bytecode = false;
nit - remove the i::FLAG_print_bytecode = false? It should be false by
default and if you don't force it to false then you can pass
--print_bytecode to the tests if you want to see the bytecode for a
particular test.

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode53
test/cctest/interpreter/test-bytecode-generator.cc:53:
nit - extra newline

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode112
test/cctest/interpreter/test-bytecode-generator.cc:112: }}};
indentation seems a little unusual here - is git cl format
responsable?

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode123
test/cctest/interpreter/test-bytecode-generator.cc:123: }
nit - extra newline below

https://codereview.chromium.org/1294543002/diff/30001/test/cctest/interpreter/test-bytecode-generator.cc#newcode124
test/cctest/interpreter/test-bytecode-generator.cc:124: }
nit - comments on closing namespace brackets.

https://codereview.chromium.org/1294543002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Changes to interpreter builtins for accumulator and register file registers. (issue 1289863003 by rmcil...@chromium.org)

[rmcilroy]

Ping?

https://codereview.chromium.org/1289863003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Allow verification and trace-turbo for bytecode handlers. (issue 1297203002 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: titzer,

Message:
These are the changes needed to print out bytecode handlers to the turbofan
visualizer. I'm happy to do this differently if you need similar things for  
WASM

(e.g., you were talking about using an enum for the pipeline steps taken in
today's meeting).

PTAL, thanks.

Description:
[Interpreter] Allow verification and trace-turbo for bytecode handlers.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1297203002/

Base URL:  
https://chromium.googlesource.com/v8/v8.git@interpreter_immed_bytecodes


Affected files (+93, -39 lines):
  M src/compiler.h
  M src/compiler.cc
  M src/compiler/arm/code-generator-arm.cc
  M src/compiler/arm64/code-generator-arm64.cc
  M src/compiler/code-generator.cc
  M src/compiler/graph-visualizer.cc
  M src/compiler/ia32/code-generator-ia32.cc
  M src/compiler/interpreter-assembler.cc
  M src/compiler/mips/code-generator-mips.cc
  M src/compiler/mips64/code-generator-mips64.cc
  M src/compiler/pipeline.h
  M src/compiler/pipeline.cc
  M src/compiler/ppc/code-generator-ppc.cc
  M src/compiler/raw-machine-assembler.cc
  M src/compiler/x64/code-generator-x64.cc
  M src/compiler/x87/code-generator-x87.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Changes to interpreter builtins for accumulator and register file registers. (issue 1289863003 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1289863003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Changes to interpreter builtins for accumulator and register file registers. (issue 1289863003 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1289863003/diff/60001/src/compiler/interpreter-assembler.h
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1289863003/diff/60001/src/compiler/interpreter-assembler.h#newcode96
src/compiler/interpreter-assembler.h:96: Node* outgoing_accumulator_;
On 2015/08/18 08:59:00, titzer wrote:

Just accumulator_?


Done.

https://codereview.chromium.org/1289863003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add implementations for load immediate bytecodes. (issue 1294793002 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1294793002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Add implementations for load immediate bytecodes. (issue 1294793002 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interpreter-assembler.cc
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interpreter-assembler.cc#newcode137
src/compiler/interpreter-assembler.cc:137: Node*
InterpreterAssembler::BytecodeOperandImm8(int delta) {
On 2015/08/17 10:49:02, oth wrote:

Suggest renaming delta for clarity - operand_index/operand_number.


Done.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interpreter-assembler.h
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interpreter-assembler.h#newcode5
src/compiler/interpreter-assembler.h:5: #ifndef
V8_COMPILER_INTERPRETER_CODEGEN_H_
On 2015/08/17 10:49:02, oth wrote:

Aaron Gray correctly highlighted the need for:
s/INTERPRETER_CODEGEN/INTERPRETER_ASSEMBLER/



Done.

https://codereview.chromium.org/1294793002/diff/20001/src/compiler/interpreter-assembler.h#newcode39
src/compiler/interpreter-assembler.h:39: // Returns the Imm8 immediate
for bytecode operand |index| in the current
On 2015/08/17 08:22:34, Michael Starzinger wrote:

nit: Comment seems to be outdated, no index is passed in.


Changed parameter and comment to operand_index

https://codereview.chromium.org/1294793002/diff/20001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1294793002/diff/20001/src/interpreter/interpreter.cc#newcode149
src/interpreter/interpreter.cc:149: __ StoreRegister(__
GetAccumulator(), __ BytecodeOperandReg(0));
On 2015/08/17 08:22:34, Michael Starzinger wrote:

This pattern looks dangerous, note that C++ does not specify the

evaluation

order or the argument, so here either GetAccumulator or

BytecodeOperandReg

could be assembled first into the bytecode stream.


Yeah good point. Note, in this case it doesn't matter for correctness
which argument is evaluated first, but agree that this could be
confusing and potentially dangerous later. Fixed to define the order.

https://codereview.chromium.org/1294793002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Minimal bytecode generator. (issue 1294543002 by o...@chromium.org)

[rmcilroy]

Looks great to me (once tests are added). Maybe let Michael have a look too?

For tests, how about adding a cctest (we probably do this as a unittests)  
which

compiles JS source and checks if the resulting bytecode is as expected?


https://codereview.chromium.org/1294543002/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1294543002/diff/1/src/flag-definitions.h#newcode284
src/flag-definitions.h:284: DEFINE_BOOL(print_ignition_bytecode, false,
print generated bytecode)
nit - just print_bytecode and add Ignition to the description?

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-generator.cc
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-generator.cc#newcode75
src/interpreter/bytecode-generator.cc:75: // Details stored in scope, ie
variable index.
/s/ie/i.e.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-generator.cc#newcode202
src/interpreter/bytecode-generator.cc:202:
DCHECK(!expr-IsPropertyName());
Could you make this an if/else or switch statement with the propertyName
case being unimplemented().

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-generator.cc#newcode239
src/interpreter/bytecode-generator.cc:239: DCHECK(variable-location()
== VariableLocation::LOCAL);
Could you do this as a switch statement, with all other cases falling
through to undefined for now.

https://codereview.chromium.org/1294543002/diff/1/src/interpreter/bytecode-generator.cc#newcode268
src/interpreter/bytecode-generator.cc:268:
builder().StoreAccumulatorInRegister(Register(destination));
We would only be storing the value in a register if it is a VARIABLE
case, wouldn't we? Could you move the switch down so this is part of the
VARIABLE case and destination can be a Register instead of an index?

https://codereview.chromium.org/1294543002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Changes to interpreter builtins for accumulator and register file registers. (issue 1289863003 by rmcil...@chromium.org)

[rmcilroy]

Thanks for the review. Comments addressed, PTAL.


https://codereview.chromium.org/1289863003/diff/20001/src/compiler/interpreter-assembler.h
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1289863003/diff/20001/src/compiler/interpreter-assembler.h#newcode40
src/compiler/interpreter-assembler.h:40: Node* IncomingAccumulator();
On 2015/08/14 12:42:20, titzer wrote:

The distinction between incoming accumulator and the outgoing

accumulator is

somewhat confusing. E.g. it really only makes sense from the

perspective of the

user of the assembler. Maybe just GetAccumulator() and

SetAccumulator(), since

the assembler is just holding the current state?


Good idea. Done.

https://codereview.chromium.org/1289863003/diff/20001/src/compiler/interpreter-assembler.h#newcode71
src/compiler/interpreter-assembler.h:71: Node* RegisterFilePointer();
On 2015/08/14 12:42:20, titzer wrote:

Can we name this RegisterFileRawPointer() and the one below
BytecodeArrayTaggedPointer() to keep them straight?


Done (also done for DispatchTablePointer below)

https://codereview.chromium.org/1289863003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [Interpreter] Add implementations for load immediate bytecodes. (issue 1294793002 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Michael Starzinger, oth,

Message:
Michael, Orion, could you take a look please? Thanks.

Description:
[Interpreter] Add implementations for load immediate bytecodes.

Adds implementations and tests for the following bytecodes:
  - LdaZero
  - LdaSmi8
  - LdaUndefined
  - LdaNull
  - LdaTheHole
  - LdaTrue
  - LdaFalse
  - LdaLdar
  - LdaStar

Also adds  Smi tagging / untagging and OperandType typed
BytecodeOperand operations to InterpreterAssembler.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1294793002/

Base URL: https://chromium.googlesource.com/v8/v8.git@interpreter_accum

Affected files (+274, -70 lines):
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/compiler/interpreter-assembler-unittest.h
  M test/unittests/compiler/interpreter-assembler-unittest.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [Interpreter] Move interpreter initialization until after snapshot deserialization. (issue 1290883004 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Hannes Payer,

Message:
Hannes: could you please take a look, thanks.

Description:
[Interpreter] Move interpreter initialization until after snapshot
deserialization.

The interpreter needs to be initialized after the snapshot has been
deserialized.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1290883004/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+4, -4 lines):
  M src/isolate.cc


Index: src/isolate.cc
diff --git a/src/isolate.cc b/src/isolate.cc
index  
47ab368dcc6acd3c30571d683e4d7fa606e5cfdc..980c95d58aa4dafe183d3ab436cb45d2b694b4e6  
100644

--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -2167,10 +2167,6 @@ bool Isolate::Init(Deserializer* des) {
   bootstrapper_-Initialize(create_heap_objects);
   builtins_.SetUp(this, create_heap_objects);

-  if (FLAG_ignition) {
-interpreter_-Initialize();
-  }
-
   if (FLAG_log_internal_timer_events) {
 set_event_logger(Logger::DefaultEventLoggerSentinel);
   }
@@ -2197,6 +2193,10 @@ bool Isolate::Init(Deserializer* des) {
   }
   stub_cache_-Initialize();

+  if (FLAG_ignition) {
+interpreter_-Initialize();
+  }
+
   // Finish initialization of ThreadLocal after deserialization is done.
   clear_pending_exception();
   clear_pending_message();


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [interpreter]: Changes to interpreter builtins for accumulator and register file registers. (issue 1289863003 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: titzer,

Message:
Ben: Could you take a look please? Thanks.

Description:
[interpreter]: Changes to interpreter builtins for accumulator and register  
file

registers.

Makes the following modifications to the interpreter builtins and
InterpreterAssembler:
 - Adds an accumulator register and initializes it to undefined()
 - Adds a register file pointer register and use it instead of FramePointer  
to

   access registers
 - Modifies builtin to support functions with 0 regiters in the register  
file

 - Modifies builtin to Call rather than TailCall to first bytecode handler.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1289863003/

Base URL:  
https://chromium.googlesource.com/v8/v8.git@fix_interpreter_initialization


Affected files (+333, -196 lines):
  M src/arm/builtins-arm.cc
  M src/arm/macro-assembler-arm.h
  M src/arm64/builtins-arm64.cc
  M src/arm64/macro-assembler-arm64.h
  M src/compiler/interpreter-assembler.h
  M src/compiler/interpreter-assembler.cc
  M src/compiler/linkage.h
  M src/compiler/linkage.cc
  M src/compiler/raw-machine-assembler.h
  M src/compiler/raw-machine-assembler.cc
  M src/ia32/builtins-ia32.cc
  M src/ia32/macro-assembler-ia32.h
  M src/mips/builtins-mips.cc
  M src/mips/macro-assembler-mips.h
  M src/mips64/assembler-mips64.h
  M src/mips64/builtins-mips64.cc
  M src/mips64/macro-assembler-mips64.h
  M src/x64/builtins-x64.cc
  M src/x64/macro-assembler-x64.h
  M test/cctest/interpreter/test-interpreter.cc
  M test/unittests/compiler/interpreter-assembler-unittest.h
  M test/unittests/compiler/interpreter-assembler-unittest.cc
  M test/unittests/compiler/node-test-utils.h
  M test/unittests/compiler/node-test-utils.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: MIPS64: Fix InterpreterEntryTrampoline(). (issue 1286343002 by paul.l...@imgtec.com)

[rmcilroy]

On 2015/08/13 04:38:58, paul.l... wrote:
Fix a mistake in my porting of https://codereview.chromium.org/1245133002  
for

mips. PTAL.


Lgtm, thanks!

https://codereview.chromium.org/1286343002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Update BytecodeArrayBuilder register handling. (issue 1283313003 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: oth,

Message:
Orion: could you take a look please? Thanks.

Description:
[interpreter]: Update BytecodeArrayBuilder register handling.

Modifies the BytecodeArrayBuilder to create register operands which are
negative. This reduces the number of instructions to access registers
by the interpreter and allows us to use positive register operands to
access parameter values.

Adds a Register class to keep register usage typesafe and simplify the
convertion to bytecode operand values.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1283313003/

Base URL:  
ssh://rmcilroy.lon.corp.google.com///usr/local/google/code/v8_full/v8@master


Affected files (+107, -160 lines):
  M src/interpreter/bytecode-array-builder.h
  M src/interpreter/bytecode-array-builder.cc
  M src/interpreter/bytecodes.h
  M src/interpreter/bytecodes.cc
  M test/cctest/cctest.gyp
  D test/cctest/interpreter/test-bytecode-array-builder.cc
  A + test/unittests/interpreter/bytecode-array-builder-unittest.cc
  M test/unittests/unittests.gyp


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Update BytecodeArrayBuilder register handling. (issue 1283313003 by rmcil...@chromium.org)

[rmcilroy]

Micheal: could you stamp for committer/OWNERS please? Thanks.

https://codereview.chromium.org/1283313003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Update BytecodeArrayBuilder register handling. (issue 1283313003 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1283313003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Update BytecodeArrayBuilder register handling. (issue 1283313003 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1283313003/diff/40001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1283313003/diff/40001/src/interpreter/bytecode-array-builder.h#newcode82
src/interpreter/bytecode-array-builder.h:82: DCHECK_LE(index_,
kMaxRegisterIndex);
On 2015/08/13 10:29:55, oth wrote:

Probably worth having a lower bounds DCHECK too.


Done.

https://codereview.chromium.org/1283313003/diff/40001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1283313003/diff/40001/src/interpreter/bytecodes.h#newcode113
src/interpreter/bytecodes.h:113: std::ostream operator(std::ostream
os, const OperandType operand_type);
On 2015/08/13 10:29:55, oth wrote:

Couldn't spot where this is used (?).


It's required by DCHECK_EQ(x, OperandType::x). I just noticed that I
didn't add the DCHECK in this CL but in a subsequent CL, but given I'm
touching these files here I'd prefer just to keep in in this CL if
that's OK with you?

https://codereview.chromium.org/1283313003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [interpreter]: Fix interpreter handler table initialization. (issue 1288893003 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: oth, Hannes Payer,

Message:
Orion: could you take a look please?

Hannes: could you stamp for OWNER/committer (any comments welcome too).

Description:
[interpreter]: Fix interpreter handler table initialization.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1288893003/

Base URL:  
https://chromium.googlesource.com/v8/v8.git@interpreter_bytecode_regs


Affected files (+1, -5 lines):
  M src/interpreter/interpreter.cc


Index: src/interpreter/interpreter.cc
diff --git a/src/interpreter/interpreter.cc b/src/interpreter/interpreter.cc
index  
50abb0f1a290053782082ad52f0cce149d0876bb..4f961318085e12bbf63f5d336d006ddda14c909d  
100644

--- a/src/interpreter/interpreter.cc
+++ b/src/interpreter/interpreter.cc
@@ -31,9 +31,6 @@ HandleFixedArray  
Interpreter::CreateUninitializedInterpreterTable(

   // it was allocated on the first page (which is always immovable).
   DCHECK(isolate-heap()-old_space()-FirstPage()-Contains(
   handler_table-address()));
-  for (int i = 0; i  static_castint(Bytecode::kLast); i++) {
-handler_table-set(i,  
isolate-builtins()-builtin(Builtins::kIllegal));

-  }
   return handler_table;
 }

@@ -62,8 +59,7 @@ void Interpreter::Initialize() {
 bool Interpreter::IsInterpreterTableInitialized(
 HandleFixedArray handler_table) {
   DCHECK(handler_table-length() == static_castint(Bytecode::kLast) + 1);
-  return handler_table-get(0) ==
- isolate_-builtins()-builtin(Builtins::kIllegal);
+  return handler_table-get(0) != isolate_-heap()-undefined_value();
 }




--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Fix interpreter handler table initialization. (issue 1288893003 by rmcil...@chromium.org)

[rmcilroy]

To give some background on why this is needed - turns out the builtins  
aren't

generated at the time CreateUninitializedInterpreterTable is called.

https://codereview.chromium.org/1288893003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Fix interpreter handler table initialization. (issue 1288893003 by rmcil...@chromium.org)

[rmcilroy]

lgtm.

https://codereview.chromium.org/1288893003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter]: Fix interpreter handler table initialization. (issue 1288893003 by rmcil...@chromium.org)

[rmcilroy]

On 2015/08/13 13:59:32, rmcilroy wrote:

lgtm.


Hannes: could you stamp for OWNERS please?

https://codereview.chromium.org/1288893003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Register conversion fix and test. (issue 1294523002 by o...@chromium.org)

[rmcilroy]

On 2015/08/13 15:54:24, oth wrote:

Ross, PTAL, thanks!


lgtm, thanks for the fix!

https://codereview.chromium.org/1294523002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Fix nosnap build for interpreter table generation. (issue 1278413002 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1278413002/diff/40001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1278413002/diff/40001/src/interpreter/interpreter.cc#newcode43
src/interpreter/interpreter.cc:43: if (initialized_) return;
On 2015/08/10 16:28:05, Hannes Payer wrote:

What is the purpose of initialized_?


It was for the code in test-interpreter to check whether it was
initialized before calling initialize again, but agree, this is no
longer useful - removed.

https://codereview.chromium.org/1278413002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Fix nosnap build for interpreter table generation. (issue 1278413002 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1278413002/diff/60001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1278413002/diff/60001/src/interpreter/interpreter.cc#newcode41
src/interpreter/interpreter.cc:41: void Interpreter::Initialize(bool
create_heap_objects) {
On 2015/08/10 16:59:25, Hannes Payer wrote:

On 2015/08/10 16:54:36, rmcilroy wrote:
 On 2015/08/10 16:50:00, Hannes Payer wrote:
  Why would you call this function with create_heap_objects=false?

 It is called with create_heap_objects=false in Isolate::Init() if

the

 deserializer is non-null. We could possibly drop this for now (since

it still

 needs to check with IsInterpreterTableInitialized anyway) but I

would like to

 keep it for the time when the flag is enabled by default and we can

remove

 IsInterpreterTableInitialized (and only rely on create_heap_objects

to decide

 whether we need to generate the interpreter table).



Sure, but the semantics right now is strange. It is more like
force_create_heap_objects since it is happily ignored by the ||

condition. I

would remove it for now. You can always bring it back.


Ok, done.

https://codereview.chromium.org/1278413002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Fix nosnap build for interpreter table generation. (issue 1278413002 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1278413002/diff/60001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1278413002/diff/60001/src/interpreter/interpreter.cc#newcode41
src/interpreter/interpreter.cc:41: void Interpreter::Initialize(bool
create_heap_objects) {
On 2015/08/10 16:50:00, Hannes Payer wrote:

Why would you call this function with create_heap_objects=false?


It is called with create_heap_objects=false in Isolate::Init() if the
deserializer is non-null. We could possibly drop this for now (since it
still needs to check with IsInterpreterTableInitialized anyway) but I
would like to keep it for the time when the flag is enabled by default
and we can remove IsInterpreterTableInitialized (and only rely on
create_heap_objects to decide whether we need to generate the
interpreter table).

https://codereview.chromium.org/1278413002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [interpreter] Fix nosnap build for interpreter table generation. (issue 1278413002 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Hannes Payer,

Message:
Hannes, could you have a quick look please (this is needed to green the  
tree).

Thanks!

Description:
[interpreter] Fix nosnap build for interpreter table generation.

Moves the creation of the interpreter table early on during initialization
to ensure that even on nosnap builds it still gets allocated in the
first page.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1278413002/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+47, -27 lines):
  M src/heap/heap.h
  M src/heap/heap.cc
  M src/interpreter/interpreter.h
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-interpreter.cc


Index: src/heap/heap.cc
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
index  
eb18925b20e6b430fb18d89038195fe496fe9673..225fd89bd5b8dd4e9df4346ee54b23a8df0869cf  
100644

--- a/src/heap/heap.cc
+++ b/src/heap/heap.cc
@@ -25,6 +25,7 @@
 #include src/heap/objects-visiting.h
 #include src/heap/store-buffer.h
 #include src/heap-profiler.h
+#include src/interpreter/interpreter.h
 #include src/runtime-profiler.h
 #include src/scopeinfo.h
 #include src/snapshot/natives.h
@@ -3362,7 +3363,9 @@ void Heap::CreateInitialObjects() {
   set_weak_stack_trace_list(Smi::FromInt(0));

   // Will be filled in by Interpreter::Initialize().
-  set_interpreter_table(empty_fixed_array());
+  set_interpreter_table(
+  *interpreter::Interpreter::CreateUninitializedInterpreterTable(
+  isolate()));

   set_allocation_sites_scratchpad(
   *factory-NewFixedArray(kAllocationSiteScratchpadSize, TENURED));
@@ -3414,7 +3417,6 @@ bool  
Heap::RootCanBeWrittenAfterInitialization(Heap::RootListIndex root_index) {

 case kWeakObjectToCodeTableRootIndex:
 case kRetainedMapsRootIndex:
 case kWeakStackTraceListRootIndex:
-case kInterpreterTableRootIndex:
 // Smi values
 #define SMI_ENTRY(type, name, Name) case k##Name##RootIndex:
   SMI_ROOT_LIST(SMI_ENTRY)
Index: src/heap/heap.h
diff --git a/src/heap/heap.h b/src/heap/heap.h
index  
561d24cc2ff9e0a6f934c0c969882c4c1d86f3c5..4a1c1f8b1faa90eb5b08f9e3fd9eaab90378dbf6  
100644

--- a/src/heap/heap.h
+++ b/src/heap/heap.h
@@ -1024,10 +1024,6 @@ class Heap {
 roots_[kMaterializedObjectsRootIndex] = objects;
   }

-  void public_set_interpreter_table(FixedArray* table) {
-roots_[kInterpreterTableRootIndex] = table;
-  }
-
   // Generated code can embed this address to get access to the roots.
   Object** roots_array_start() { return roots_; }

Index: src/interpreter/interpreter.cc
diff --git a/src/interpreter/interpreter.cc b/src/interpreter/interpreter.cc
index  
3d37874075575fcda79404b63d0a4f1c01713791..6ffbe48e1930024d58795236e9ac7fa3427eff64  
100644

--- a/src/interpreter/interpreter.cc
+++ b/src/interpreter/interpreter.cc
@@ -18,33 +18,48 @@ using compiler::Node;
 #define __ assembler-


-Interpreter::Interpreter(Isolate* isolate) : isolate_(isolate) {}
+Interpreter::Interpreter(Isolate* isolate)
+: isolate_(isolate), initialized_(false) {}
+
+
+// static
+HandleFixedArray Interpreter::CreateUninitializedInterpreterTable(
+Isolate* isolate) {
+  HandleFixedArray handler_table = isolate-factory()-NewFixedArray(
+  static_castint(Bytecode::kLast) + 1, TENURED);
+  // We rely on the interpreter handler table being immovable, so check  
that

+  // it was allocated on the first page (which is always immovable).
+  DCHECK(isolate-heap()-old_space()-FirstPage()-Contains(
+  handler_table-address()));
+  for (int i = 0; i  static_castint(Bytecode::kLast); i++) {
+handler_table-set(i,  
isolate-builtins()-builtin(Builtins::kIllegal));

+  }
+  return handler_table;
+}


 void Interpreter::Initialize(bool create_heap_objects) {
   DCHECK(FLAG_ignition);
+  if (initialized_) return;
+
   if (create_heap_objects) {
 Zone zone;
 HandleScope scope(isolate_);
-HandleFixedArray handler_table = isolate_-factory()-NewFixedArray(
-static_castint(Bytecode::kLast) + 1, TENURED);
-// We rely on the interpreter handler table being immovable, so check  
that

-// it was allocated on the first page (which is always immovable).
-DCHECK(isolate_-heap()-old_space()-FirstPage()-Contains(
-handler_table-address()));
-isolate_-heap()-public_set_interpreter_table(*handler_table);
-
-#define GENERATE_CODE(Name, ...)\
-  { \
-compiler::InterpreterAssembler assembler(isolate_, zone,   \
- Bytecode::k##Name);\
-Do##Name(assembler);   \
-HandleCode code = assembler.GenerateCode();   \
-handler_table-set(static_castint(Bytecode::k##Name), *code); \
-  }
+HandleFixedArray handler_table =  
isolate_-factory()-interpreter_table();

+
+#define GENERATE_CODE(Name, ...)  \
+

[v8-dev] [interpreter] Fix nosnap build for interpreter table generation. (issue 1278413002 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: Hannes Payer,

Message:
Hannes, could you have a quick look please (this is needed to green the  
tree).

Thanks!

Description:
[interpreter] Fix nosnap build for interpreter table generation.

Moves the creation of the interpreter table early on during initialization
to ensure that even on nosnap builds it still gets allocated in the
first page.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1278413002/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+47, -27 lines):
  M src/heap/heap.h
  M src/heap/heap.cc
  M src/interpreter/interpreter.h
  M src/interpreter/interpreter.cc
  M test/cctest/interpreter/test-interpreter.cc


Index: src/heap/heap.cc
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
index  
eb18925b20e6b430fb18d89038195fe496fe9673..225fd89bd5b8dd4e9df4346ee54b23a8df0869cf  
100644

--- a/src/heap/heap.cc
+++ b/src/heap/heap.cc
@@ -25,6 +25,7 @@
 #include src/heap/objects-visiting.h
 #include src/heap/store-buffer.h
 #include src/heap-profiler.h
+#include src/interpreter/interpreter.h
 #include src/runtime-profiler.h
 #include src/scopeinfo.h
 #include src/snapshot/natives.h
@@ -3362,7 +3363,9 @@ void Heap::CreateInitialObjects() {
   set_weak_stack_trace_list(Smi::FromInt(0));

   // Will be filled in by Interpreter::Initialize().
-  set_interpreter_table(empty_fixed_array());
+  set_interpreter_table(
+  *interpreter::Interpreter::CreateUninitializedInterpreterTable(
+  isolate()));

   set_allocation_sites_scratchpad(
   *factory-NewFixedArray(kAllocationSiteScratchpadSize, TENURED));
@@ -3414,7 +3417,6 @@ bool  
Heap::RootCanBeWrittenAfterInitialization(Heap::RootListIndex root_index) {

 case kWeakObjectToCodeTableRootIndex:
 case kRetainedMapsRootIndex:
 case kWeakStackTraceListRootIndex:
-case kInterpreterTableRootIndex:
 // Smi values
 #define SMI_ENTRY(type, name, Name) case k##Name##RootIndex:
   SMI_ROOT_LIST(SMI_ENTRY)
Index: src/heap/heap.h
diff --git a/src/heap/heap.h b/src/heap/heap.h
index  
561d24cc2ff9e0a6f934c0c969882c4c1d86f3c5..4a1c1f8b1faa90eb5b08f9e3fd9eaab90378dbf6  
100644

--- a/src/heap/heap.h
+++ b/src/heap/heap.h
@@ -1024,10 +1024,6 @@ class Heap {
 roots_[kMaterializedObjectsRootIndex] = objects;
   }

-  void public_set_interpreter_table(FixedArray* table) {
-roots_[kInterpreterTableRootIndex] = table;
-  }
-
   // Generated code can embed this address to get access to the roots.
   Object** roots_array_start() { return roots_; }

Index: src/interpreter/interpreter.cc
diff --git a/src/interpreter/interpreter.cc b/src/interpreter/interpreter.cc
index  
3d37874075575fcda79404b63d0a4f1c01713791..6ffbe48e1930024d58795236e9ac7fa3427eff64  
100644

--- a/src/interpreter/interpreter.cc
+++ b/src/interpreter/interpreter.cc
@@ -18,33 +18,48 @@ using compiler::Node;
 #define __ assembler-


-Interpreter::Interpreter(Isolate* isolate) : isolate_(isolate) {}
+Interpreter::Interpreter(Isolate* isolate)
+: isolate_(isolate), initialized_(false) {}
+
+
+// static
+HandleFixedArray Interpreter::CreateUninitializedInterpreterTable(
+Isolate* isolate) {
+  HandleFixedArray handler_table = isolate-factory()-NewFixedArray(
+  static_castint(Bytecode::kLast) + 1, TENURED);
+  // We rely on the interpreter handler table being immovable, so check  
that

+  // it was allocated on the first page (which is always immovable).
+  DCHECK(isolate-heap()-old_space()-FirstPage()-Contains(
+  handler_table-address()));
+  for (int i = 0; i  static_castint(Bytecode::kLast); i++) {
+handler_table-set(i,  
isolate-builtins()-builtin(Builtins::kIllegal));

+  }
+  return handler_table;
+}


 void Interpreter::Initialize(bool create_heap_objects) {
   DCHECK(FLAG_ignition);
+  if (initialized_) return;
+
   if (create_heap_objects) {
 Zone zone;
 HandleScope scope(isolate_);
-HandleFixedArray handler_table = isolate_-factory()-NewFixedArray(
-static_castint(Bytecode::kLast) + 1, TENURED);
-// We rely on the interpreter handler table being immovable, so check  
that

-// it was allocated on the first page (which is always immovable).
-DCHECK(isolate_-heap()-old_space()-FirstPage()-Contains(
-handler_table-address()));
-isolate_-heap()-public_set_interpreter_table(*handler_table);
-
-#define GENERATE_CODE(Name, ...)\
-  { \
-compiler::InterpreterAssembler assembler(isolate_, zone,   \
- Bytecode::k##Name);\
-Do##Name(assembler);   \
-HandleCode code = assembler.GenerateCode();   \
-handler_table-set(static_castint(Bytecode::k##Name), *code); \
-  }
+HandleFixedArray handler_table =  
isolate_-factory()-interpreter_table();

+
+#define GENERATE_CODE(Name, ...)  \
+

[v8-dev] Re: [interpreter] Adds interpreter cctests. (issue 1269683002 by rmcil...@chromium.org)

[rmcilroy]

PTAL, thanks.


https://codereview.chromium.org/1269683002/diff/60001/test/cctest/interpreter/test-interpreter.cc
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1269683002/diff/60001/test/cctest/interpreter/test-interpreter.cc#newcode31
test/cctest/interpreter/test-interpreter.cc:31: MaybeHandleObject
Call() {
On 2015/07/31 12:09:12, titzer wrote:

Can we factor the Call() out of the tester?



This is one of the design mistakes I think we made with the way the

other

integration tests work. The tester should produce a Callable thingy,

which has a

Call() or an operator()


Done.

https://codereview.chromium.org/1269683002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Adds interpreter cctests. (issue 1269683002 by rmcil...@chromium.org)

[rmcilroy]

Ben, could you take a look please, thanks.

https://codereview.chromium.org/1269683002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Adds interpreter cctests. (issue 1269683002 by rmcil...@chromium.org)

[rmcilroy]

Ben, could you take a look please, thanks.

https://codereview.chromium.org/1269683002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [turbofan] Fix kArchTailCallCodeObject on ia32/x64. (issue 1265723003 by rmcil...@chromium.org)

[rmcilroy]

Reviewers: titzer,

Message:
PTAL. As discussed, this will be tested
byhttps://codereview.chromium.org/1269683002/, but I will add an explicit  
test

when I return from vacation on Aug 10th.

Description:
[turbofan] Fix kArchTailCallCodeObject on ia32/x64.

Previously these instructions tried to jump to the value at the code entry's
location, rather than jumping to this location. Also adds a test.

BUG=v8:4280
LOG=N

Please review this at https://codereview.chromium.org/1265723003/

Base URL:  
ssh://rmcilroy.lon.corp.google.com///usr/local/google/code/v8_full/v8@inter_linkage_change


Affected files (+4, -3 lines):
  M src/compiler/ia32/code-generator-ia32.cc
  M src/compiler/x64/code-generator-x64.cc


Index: src/compiler/ia32/code-generator-ia32.cc
diff --git a/src/compiler/ia32/code-generator-ia32.cc  
b/src/compiler/ia32/code-generator-ia32.cc
index  
4690a8cc05d869893778ed9f12c229b2b10f88c5..12eb42448609fad66a009c6d8116812921283bb0  
100644

--- a/src/compiler/ia32/code-generator-ia32.cc
+++ b/src/compiler/ia32/code-generator-ia32.cc
@@ -318,7 +318,8 @@ void  
CodeGenerator::AssembleArchInstruction(Instruction* instr) {

 __ jmp(code, RelocInfo::CODE_TARGET);
   } else {
 Register reg = i.InputRegister(0);
-__ jmp(Operand(reg, Code::kHeaderSize - kHeapObjectTag));
+__ add(reg, Immediate(Code::kHeaderSize - kHeapObjectTag));
+__ jmp(reg);
   }
   break;
 }
Index: src/compiler/x64/code-generator-x64.cc
diff --git a/src/compiler/x64/code-generator-x64.cc  
b/src/compiler/x64/code-generator-x64.cc
index  
bdce0832011e4ce8a1c21a301c1672535e7f7171..56477d66b66c9baa0d527da7ed3554d1d69f4e0a  
100644

--- a/src/compiler/x64/code-generator-x64.cc
+++ b/src/compiler/x64/code-generator-x64.cc
@@ -567,8 +567,8 @@ void  
CodeGenerator::AssembleArchInstruction(Instruction* instr) {

 __ jmp(code, RelocInfo::CODE_TARGET);
   } else {
 Register reg = i.InputRegister(0);
-int entry = Code::kHeaderSize - kHeapObjectTag;
-__ jmp(Operand(reg, entry));
+__ addp(reg, Immediate(Code::kHeaderSize - kHeapObjectTag));
+__ jmp(reg);
   }
   break;
 }


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Intepreter] Naive bytecode generation local variable assignment and +/*-. (issue 1266713004 by o...@chromium.org)

[rmcilroy]

Also:
  - Update title to mention this also adds a BytecodeArrayBuilder
  - Update description to have the same first line as title (this is what
actually get's used in the commit log).
  - Add a bit more detail in the description
  - Would be nice to have some simple unittests for the  
BytecodeArrayBuilder (in

test/unittest/interpreter)

Other than that, looks great - lgtm once the comments and above are done.


https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1266713004/diff/20001/src/interpreter/bytecode-array-builder.cc#newcode76
src/interpreter/bytecode-array-builder.cc:76: int
BytecodeArrayBuilder::AllocateScratchRegister() {
On 2015/07/30 15:38:42, oth wrote:

On 2015/07/30 10:12:24, rmcilroy (OOO until 10th Aug) wrote:
 These should be Pop/PushScratchRegister if they require that the

scratches are

 pushed and popped in order. Also could we call them TempRegister

instead

 (scratch is typically something which is only live for a very short

time,

where
 these might be live for the whole of an expression).



Done.


Missed rename to Push/Pop?

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.cc#newcode29
src/interpreter/bytecode-array-builder.cc:29: int frame_size =
register_count * sizeof(intptr_t);
replace sizeof(intptr_t) with kPointerSize (otherwise when cross
compiling the framesize will be wrong in the serialized snapshot).

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.h
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.h#newcode25
src/interpreter/bytecode-array-builder.h:25: // Set number of locals
required for bytecode.
s/for bytecode/by bytecode array/

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecode-array-builder.h#newcode36
src/interpreter/bytecode-array-builder.h:36: //
===
nit - this type of header format isn't very common in V8. I would just
have:
  // Constant loads to accumulator
  BytecodeArrayBuilder LoadLiteral

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/bytecodes.h#newcode28
src/interpreter/bytecodes.h:28: \
super nit - remove newline below comment.

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode51
src/interpreter/interpreter.cc:51: // LdaZero dst
Remove dst

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode53
src/interpreter/interpreter.cc:53: // Load literal '0' into the
destination register.
update comment /s/destination/accumulator/ and remove the code
generation (since it's wrong now).

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode61
src/interpreter/interpreter.cc:61: // LdaSmi8 dst, imm8
remove dst

https://codereview.chromium.org/1266713004/diff/40001/src/interpreter/interpreter.cc#newcode63
src/interpreter/interpreter.cc:63: // Load an 8-bit integer literal into
destination register as a Smi.
/s/destination/acumulator

https://codereview.chromium.org/1266713004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1266713004/diff/40001/src/objects.cc#newcode11633
src/objects.cc:11633: case interpreter::OperandType::kNone:
We should never hit this case, right? If so, maybe just add
UNREACHABLE() here (and move to the bottom)

https://codereview.chromium.org/1266713004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Interpreter] Remove unnecessary const specifiers on scalar types. (issue 1269813006 by o...@chromium.org)

[rmcilroy]

On 2015/07/31 10:01:43, oth wrote:

PTAL, thanks!


lgtm, thanks!

https://codereview.chromium.org/1269813006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Fix idle notification for background tab. (issue 1269583002 by u...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1269583002/diff/1/src/heap/gc-idle-time-handler.cc
File src/heap/gc-idle-time-handler.cc (right):

https://codereview.chromium.org/1269583002/diff/1/src/heap/gc-idle-time-handler.cc#newcode205
src/heap/gc-idle-time-handler.cc:205:
idle_times_which_made_no_progress_++;
On 2015/07/29 18:21:30, ulan wrote:

On 2015/07/29 18:11:04, rmcilroy (OOO until 10th Aug) wrote:
 Could this cause wrapping of idle_times_which_made_no_progress_?

Maybe clap

 idle_times_which_made_no_progress_ to be less than or equal to
 kMaxNoProgressIdleTimes?
Thanks, rewrote to clap the counter.
Interval between background idle notifications is at least 1 second,

so wrapping

can happen in 60 years. :)


Better safe than sorry ;). Thanks.

https://codereview.chromium.org/1269583002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [interpreter] Add Interpreter{Entry,Exit}Trampoline builtins. (issue 1245133002 by rmcil...@chromium.org)

[rmcilroy]

https://codereview.chromium.org/1245133002/diff/11/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/1245133002/diff/11/src/arm/builtins-arm.cc#newcode951
src/arm/builtins-arm.cc:951: //  - Allocating a new local context if
applicable.
On 2015/07/28 13:06:53, Benedikt Meurer wrote:

Note: This should be explicit as in TurboFan.


Agreed, this was already my plan :). Rearranged the bullet points to
make it clear which are likely to be explicit bytecodes, and which will
be handled in the entry stub.

https://codereview.chromium.org/1245133002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.