Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398023002:
[wasm] asm.js - Parse and convert asm.js to wasm a function at a time. (Closed)
|
Created:
4 years, 2 months ago by bradnelson Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] asm.js - Parse and convert asm.js to wasm a function at a time.
Make the AsmWasmBuilder drive the process of typing and potentially parsing
function bodies. This will allow us to keep only a single asm.js function's
AST in memory as we convert to WebAssembly.
This is needed to keep our memory footprint low.
Add some additional output to a few tests that's helpful to see which stage they fail at.
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203
LOG=N
R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromium.org
Committed: https://crrev.com/14e05c104684226ecc2ecaef9794d55803f52023
Cr-Commit-Position: refs/heads/master@{#41372}
Patch Set 1 #
Total comments: 6
Patch Set 2 : merge #Patch Set 3 : fixes #Patch Set 4 : fix #Patch Set 5 : drop junk #Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : drop old #Patch Set 9 : drop leftover #
Total comments: 8
Patch Set 10 : revised #
Total comments: 24
Patch Set 11 : git cl try #
Total comments: 2
Patch Set 12 : fix #Patch Set 13 : clear function node each time #
Total comments: 8
Patch Set 14 : review fixes + fix asan forward reference failure #
Total comments: 4
Patch Set 15 : Fix DCHECKS #
Total comments: 17
Patch Set 16 : fix #Patch Set 17 : fix #
Messages
Total messages: 92 (63 generated)
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-typer.h File src/asmjs/asm-typer.h (right): https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-typer.h#newco... src/asmjs/asm-typer.h:254: AsmType* ValidateModulePhase1of2(FunctionLiteral* fun); Can we get some more descriptive names for these phases? https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-wasm-builder.cc File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:140: Handle<SharedFunctionInfo> shared = Do we really need a shared function info here? Seems wasteful. https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:504: if (scope_ == kFuncScope) { Why do we need to do the declarations after for a func scope?
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
Description was changed from ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=adamk@chromium.org,aseemgarg@chromium.org,titzer@chromium.org ========== to ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... ==========
bradnelson@google.com changed reviewers: + marja@chromium.org
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bradnelson@google.com changed reviewers: + bradnelson@google.com
Still sorting thru ~12 failures or so, but would appreciate feedback. https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-typer.h File src/asmjs/asm-typer.h (right): https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-typer.h#newco... src/asmjs/asm-typer.h:254: AsmType* ValidateModulePhase1of2(FunctionLiteral* fun); On 2016/10/07 12:56:59, titzer wrote: > Can we get some more descriptive names for these phases? Done. https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-wasm-builder.cc File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:140: Handle<SharedFunctionInfo> shared = On 2016/10/07 12:56:59, titzer wrote: > Do we really need a shared function info here? Seems wasteful. A large number of places in the parser assume a SharedFunctionInfo in the case you're parsing one function. Added a TODO to refactor to avoid needing this. https://codereview.chromium.org/2398023002/diff/1/src/asmjs/asm-wasm-builder.... src/asmjs/asm-wasm-builder.cc:504: if (scope_ == kFuncScope) { On 2016/10/07 12:56:59, titzer wrote: > Why do we need to do the declarations after for a func scope? No good reason. Both should be declarations then statements (in the new traversal). Changed.
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
When do you use ParseFunction and when ParseProgram - why do you need to change them like this?
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... ========== to ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. Add some additional output to a few tests that's helpful to see which stage they fail at. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... ==========
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Marja, ah yes, I don't need to change ParseProgram, as it's not used for this (I had tried that at one point and forgot it was not longer going that path after changing it back). Thanks The tests are now all passing, but I'm not confident with several portions of this change. Here are some questions I have: * The CL refactors asm-typer to be able to run in phases, and separately does the same for asm-wasm-builder and wires it into compiler.cc. I can probably separate out the refactoring of asm-typer into a separate change, would this be helpful? * Several DCHECKs in scopes.cc seem to assert things that seem too restrictive, but I'm unsure. For instance asserting that variables are only resolved at script scope? * I ended up needing to yank a scope out of a scope chain (and make the remove method public). This is because the scope is only being used temporarily (and is in its own zone). Is this okay? * Am I re-parsing a single function correctly in asm-wasm-builder.cc (VisitFunctionDeclarations)? This is the portion of the change I'm least confident in. Guidance would be much appreciated. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Had a look at the removed DCHECKS... I don't think I understand how you're calling these functions and what exactly fails here. https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:551: // scope->outer_scope()->already_resolved_); Why is the outer scope not already_resolved_ in your use case? The ctor which constructs a Scope based on ScopeInfo sets already_resolved_ to true. So this DCHECK checks (in your case, afaics) that you call Analyze on the inner scope of such a scope... is that not the case? https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); Removing this check seems wrong - why would your CL change this? Maybe you're passing in the wrong Zone. https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1651: // DCHECK(info->script_scope()->is_script_scope()); What's the failure here? is script_scope() null or just not a script scope?
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:551: // scope->outer_scope()->already_resolved_); On 2016/11/26 15:46:36, marja wrote: > Why is the outer scope not already_resolved_ in your use case? > > The ctor which constructs a Scope based on ScopeInfo sets already_resolved_ to > true. So this DCHECK checks (in your case, afaics) that you call Analyze on the > inner scope of such a scope... is that not the case? So I'm calling this on a scope for an asm function, inside the scope for an asm module. The module is sometimes at script scope, but can be wrapped inside an eval or other function scope (as is the case in many of our tests). Does it make sense to run Analyze on such a scope? My goal is to allocate Variable* for each variable in the function, so I can use them during asm.js validation. https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); On 2016/11/26 15:46:36, marja wrote: > Removing this check seems wrong - why would your CL change this? Maybe you're > passing in the wrong Zone. So this is happening because I'm passing in an AstValueFactory for each ParseInfo that uses the zone used to parse the module (rather than using the separate zone I'm parsing each asm function in). I did this because Scopes use VariableMap which in turn seems to rely on AstValueFactory ensuring that strings with the same contents get the same pointer value. This is ensured inside AstValueFactory by keeping a string table. Since I need variable names inside an asm function to resolve with names from the module, I share a single one (not sure if this is ideal). Suggestions very welcome :-) https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1651: // DCHECK(info->script_scope()->is_script_scope()); On 2016/11/26 15:46:36, marja wrote: > What's the failure here? is script_scope() null or just not a script scope? scope_type_ = v8::internal::ScopeType::FUNCTION_SCOPE Though I see now I'm using script_scope in a different way than intended (misunderstood and thought it was the script text being parsed), changed round, added "asm_function_scope" (not sure if this is right). What I'm trying to do is force variable resolution in the scope surrounding the asm function. https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:148: info.set_ast_value_factory(ast_value_factory_); I'd rather not have to do this, but (see other comment) Scopes seem to rely on a common AstValueFactory being used for variables names. Suggestions welcome.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-typer.h File src/asmjs/asm-typer.h (right): https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-typer.h#... src/asmjs/asm-typer.h:262: AsmType* ValidateModuleBeforeFunctionsPhase(FunctionLiteral* fun); Seems a little weird to use AsmType* as the return type to indicate success/fail. Is it ever used in another way? https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:139: Handle<SharedFunctionInfo> shared = I'm wondering if we need to squirrel these shared function infos away and save them for later use with the module... https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:549: // DCHECK(scope->scope_type() == SCRIPT_SCOPE || Can you explain why this DCHECK is no longer needed? And delete it... https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:554: scope->set_should_eager_compile(); Fishy. I don't think we want to set eager compile on the inner asm.js functions... https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); delete https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1739: // DCHECK(info->script_scope()->is_script_scope()); delete https://codereview.chromium.org/2398023002/diff/180001/src/compiler.h File src/compiler.h (right): https://codereview.chromium.org/2398023002/diff/180001/src/compiler.h#newcode75 src/compiler.h:75: static Handle<SharedFunctionInfo> NewSharedFunctionInfoFromLiteral( I think this probably belongs on Factory. https://codereview.chromium.org/2398023002/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/parsing/parser.cc#... src/parsing/parser.cc:858: if (info->asm_function_scope()) { Also fishy. I am not sure why we have to manually mess with the scope chain here.
(See previous comment from bradnelson@) https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:549: // DCHECK(scope->scope_type() == SCRIPT_SCOPE || On 2016/11/28 10:40:11, titzer wrote: > Can you explain why this DCHECK is no longer needed? And delete it... I think these commented-out-DCHECKs are stuff that fails currently and it's still unclear why. They are not to be deleted (but might need an update, idk yet)
Alright, I'm slowly beginning to understand this CL. Below I suggest a solution to the Zone mismatch DCHECK, and the is_script_scope checks should just be udpated with the relevant asm scope condition. https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); Yeah, the AstValueFactory needs to be the same, if you want to resolve variables based on those strings. I'm guessing what's happening here: - You are creating a Zone in AsmWasmBuilderImpl::VisitFunctionDeclaration and giving it to ParseInfo - You're also giving the ParseInfo an AstValueFactory which is from a different Zone. - ParserBase ctor creates an AstNodeFactory based on the passed in AstValueFactory - And that's where this conflict comes from. What you're doing is fine I think, it's just a new special case which hasn't been needed so far. When this happens, the parser should just set the Zone of the AstNodeFactory to be the Zone from the ParseInfo and everything should be fine. Right? https://codereview.chromium.org/2398023002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1651: // DCHECK(info->script_scope()->is_script_scope()); On 2016/11/27 01:14:02, bradn wrote: > On 2016/11/26 15:46:36, marja wrote: > > What's the failure here? is script_scope() null or just not a script scope? > > scope_type_ = v8::internal::ScopeType::FUNCTION_SCOPE > > Though I see now I'm using script_scope in a different way than intended > (misunderstood and thought it was the script text being parsed), changed round, > added "asm_function_scope" (not sure if this is right). > What I'm trying to do is force variable resolution in the scope surrounding the > asm function. Ok, we need to think how to make the scope analysis work and what happens at the boundary between Scopes created earlier when parsing the full thing and the Scopes created now when parsing the function itself. So now you connect the Scopes via 150 // Create fresh function scope to use to parse the function in. 151 DeclarationScope* new_func_scope = new (info.zone()) DeclarationScope( 152 info.zone(), decl->fun()->scope()->outer_scope(), FUNCTION_SCOPE); 153 info.set_asm_function_scope(new_func_scope); and then you parse & analyze the function scope. What should happen when the function refers to a variable in the outer functions? You want to connect the VariableProxy into a Variable which is out there somewhere? Maybe the right thing already happens? Another thing which I think is making the DCHECKs here fail is that the situation is different from the normal "lazy parse an individual function" code path. At that point we have reconstructed the Scope chain based on ScopeInfos, so, outer scopes are ScopeInfo backed ones, and the boundary is where non-ScopeInfo-backed Scopes meet ScopeInfo backed scopes. But now it's different... https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:148: info.set_ast_value_factory(ast_value_factory_); On 2016/11/27 01:14:02, bradn wrote: > I'd rather not have to do this, but (see other comment) Scopes seem to rely on a > common AstValueFactory being used for variables names. > Suggestions welcome. This is fine (see my other comment about how th fix the Zone mismatch). https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); On 2016/11/28 10:40:11, titzer wrote: > delete No, don't delete :) https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1739: // DCHECK(info->script_scope()->is_script_scope()); On 2016/11/28 10:40:11, titzer wrote: > delete Ditto, don't delete; but I think this needs to be updated to reflect the relevant condition for you. I guess you don't have a script scope in this case. info->script_scope()->is_script_scope() ||Â info->asm_function_scope() >something something (or maybe asm_function_scope->outer_scope()->something).
bradnelson@google.com changed reviewers: + jochen@chromium.org, verwaest@chromium.org
PTAL +jochen (have I meshed well with what you're adding with function ids?) +verawest PTAL I notice that in some places AstNodeFactory uses the zone out of an AstValueFactory. Do I need to be concerned I'm still generating the bulk of per function info into a the outer zone? https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-typer.h File src/asmjs/asm-typer.h (right): https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-typer.h#... src/asmjs/asm-typer.h:262: AsmType* ValidateModuleBeforeFunctionsPhase(FunctionLiteral* fun); On 2016/11/28 10:40:11, titzer wrote: > Seems a little weird to use AsmType* as the return type to indicate > success/fail. Is it ever used in another way? Used so the FAIL macro can be shared throughout. Not sure that's a great reason. https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:139: Handle<SharedFunctionInfo> shared = On 2016/11/28 10:40:11, titzer wrote: > I'm wondering if we need to squirrel these shared function infos away and save > them for later use with the module... Agreed, will add a TODO. https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:148: info.set_ast_value_factory(ast_value_factory_); On 2016/11/28 11:47:21, marja wrote: > On 2016/11/27 01:14:02, bradn wrote: > > I'd rather not have to do this, but (see other comment) Scopes seem to rely on > a > > common AstValueFactory being used for variables names. > > Suggestions welcome. > > This is fine (see my other comment about how th fix the Zone mismatch). Ok, thanks https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:549: // DCHECK(scope->scope_type() == SCRIPT_SCOPE || On 2016/11/28 10:40:11, titzer wrote: > Can you explain why this DCHECK is no longer needed? And delete it... Added back with new case and comment. https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:549: // DCHECK(scope->scope_type() == SCRIPT_SCOPE || On 2016/11/28 11:10:17, marja wrote: > On 2016/11/28 10:40:11, titzer wrote: > > Can you explain why this DCHECK is no longer needed? And delete it... > > I think these commented-out-DCHECKs are stuff that fails currently and it's > still unclear why. They are not to be deleted (but might need an update, idk > yet) Done. https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:554: scope->set_should_eager_compile(); On 2016/11/28 10:40:11, titzer wrote: > Fishy. I don't think we want to set eager compile on the inner asm.js > functions... I think this ends up being moot in the asm case, as these scopes never get used in a normal compile. https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); On 2016/11/28 10:40:11, titzer wrote: > delete Keeping with additional clause. https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1739: // DCHECK(info->script_scope()->is_script_scope()); On 2016/11/28 10:40:11, titzer wrote: > delete Kept instead. https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1739: // DCHECK(info->script_scope()->is_script_scope()); On 2016/11/28 11:47:21, marja wrote: > On 2016/11/28 10:40:11, titzer wrote: > > delete > > Ditto, don't delete; but I think this needs to be updated to reflect the > relevant condition for you. > > I guess you don't have a script scope in this case. > > info->script_scope()->is_script_scope() || info->asm_function_scope() >something > something (or maybe asm_function_scope->outer_scope()->something). doing this above. Actually this one can be kept as is. https://codereview.chromium.org/2398023002/diff/180001/src/compiler.h File src/compiler.h (right): https://codereview.chromium.org/2398023002/diff/180001/src/compiler.h#newcode75 src/compiler.h:75: static Handle<SharedFunctionInfo> NewSharedFunctionInfoFromLiteral( On 2016/11/28 10:40:11, titzer wrote: > I think this probably belongs on Factory. Moved the private method there. https://codereview.chromium.org/2398023002/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2398023002/diff/180001/src/parsing/parser.cc#... src/parsing/parser.cc:858: if (info->asm_function_scope()) { On 2016/11/28 10:40:11, titzer wrote: > Also fishy. I am not sure why we have to manually mess with the scope chain > here. Agreed. I had tried instead to set the outer function on the ParseInfo, but DeserializeScopeChain assumes that outer scopes are fully resolved in various way: * Have slots assigned to them * Aren't going to be disconnected from the chain To be honest I'm a little fuzzy on what deserialization means in this context?
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ah good, adding some prints confirms a couple thousand bytes are allocated on the main zone per function vs MBs on per function zone.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18544) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The Zone thing is unfortunately a bit unclean (in master). E.g., it's just an implementation detail that AstNodeFactory gets its Zone from AstValueFactory. We should really pass in a Zone and AstValueFactory. But since in most use cases, the Zone is AstValueFactory's Zone, we only pass in AstValueFactory. Would my previous suggestion work? - Don't change the Zone-related DCHECK - In Parser, if the Zone from ParseInfo != Zone from AstValueFactory, reset the Zone of the AstNodeFactory to be the former. Afaics this gets Parser into the state it should be in. (Let's talk over chat again if that doesn't work / if you have other problems with this.)
https://codereview.chromium.org/2398023002/diff/200001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/200001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1011: DCHECK(factory->zone() == zone() || outer_scope()->IsAsmFunction()); I think this DCHECK should be left as is and you should pass a correct Zone.
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Figured out how to set the zone on the ast_node_factory needed. One unexpected consequence was that AstNode addresses get recycled, which the typer had been keeping between functions (wasteful), and relying on being unique, so incorrect. This is fixed now. PTAL https://codereview.chromium.org/2398023002/diff/200001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/200001/src/ast/scopes.cc#newc... src/ast/scopes.cc:1011: DCHECK(factory->zone() == zone() || outer_scope()->IsAsmFunction()); On 2016/11/29 07:39:46, marja wrote: > I think this DCHECK should be left as is and you should pass a correct Zone. Done.
Starting to look good (I'm only having a look at the interaction w/ parser & scope, not the other parts.) https://codereview.chromium.org/2398023002/diff/240001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/240001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:166: decl->scope()->RemoveInnerScope(decl->scope()->inner_scope()); This RemoveInnerScope stuff looks a bit scary. The intention is to remove new_func_scope, right? Could you - remove that explicitly - DCHECK that the thing you wanted to remove was found https://codereview.chromium.org/2398023002/diff/240001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2398023002/diff/240001/src/ast/scopes.h#newco... src/ast/scopes.h:426: void RemoveInnerScope(Scope* inner_scope) { ... add a return value here to tell whether inner_scope was found or not. https://codereview.chromium.org/2398023002/diff/240001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2398023002/diff/240001/src/parsing/parser.cc#... src/parsing/parser.cc:864: factory()->set_zone(info->zone()); I was debating with myself whether the set_zone should be inside this if. I guess it's fine; could you add here: else { DCHECK(factory()->zone() == info->zone()); } https://codereview.chromium.org/2398023002/diff/240001/src/parsing/parser.cc#... src/parsing/parser.cc:915: // Asm functions don't get IDs. Why? Is it bad if we assign IDs to them? And what does this mean for the rest of the code (not just DoParseFunction); don't we need to adapt the rest of the ID getting code too? (jochen@ can comment on that better)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2398023002/diff/240001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/240001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:166: decl->scope()->RemoveInnerScope(decl->scope()->inner_scope()); On 2016/11/29 10:20:04, marja wrote: > This RemoveInnerScope stuff looks a bit scary. > > The intention is to remove new_func_scope, right? Could you > - remove that explicitly > - DCHECK that the thing you wanted to remove was found Done. https://codereview.chromium.org/2398023002/diff/240001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2398023002/diff/240001/src/ast/scopes.h#newco... src/ast/scopes.h:426: void RemoveInnerScope(Scope* inner_scope) { On 2016/11/29 10:20:04, marja wrote: > ... add a return value here to tell whether inner_scope was found or not. Done. https://codereview.chromium.org/2398023002/diff/240001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2398023002/diff/240001/src/parsing/parser.cc#... src/parsing/parser.cc:864: factory()->set_zone(info->zone()); On 2016/11/29 10:20:04, marja wrote: > I was debating with myself whether the set_zone should be inside this if. > > I guess it's fine; could you add here: > > else { > DCHECK(factory()->zone() == info->zone()); > } Done. https://codereview.chromium.org/2398023002/diff/240001/src/parsing/parser.cc#... src/parsing/parser.cc:915: // Asm functions don't get IDs. On 2016/11/29 10:20:04, marja wrote: > Why? Is it bad if we assign IDs to them? And what does this mean for the rest of > the code (not just DoParseFunction); don't we need to adapt the rest of the ID > getting code too? (jochen@ can comment on that better) Fair point. Setting an id for them instead of this and below.
LGTM modulo the comment I had a look at the following: - parser - scopes - how they are used in AsmWasmBuilderImpl::VisitFunctionDeclaration https://codereview.chromium.org/2398023002/diff/260001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/260001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:170: DCHECK(decl->scope()->RemoveInnerScope(new_func_scope)); This is not what you wanted :) Now RemoveInnerScope will only be executed in debug mode. https://codereview.chromium.org/2398023002/diff/260001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:180: DCHECK(decl->scope()->RemoveInnerScope(new_func_scope)); Ditto
The CQ bit was checked by bradnelson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Could others chime in on the remaining bits. Thanks kindly! https://codereview.chromium.org/2398023002/diff/260001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/260001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:170: DCHECK(decl->scope()->RemoveInnerScope(new_func_scope)); On 2016/11/29 10:48:50, marja wrote: > This is not what you wanted :) > > Now RemoveInnerScope will only be executed in debug mode. Oops Done. https://codereview.chromium.org/2398023002/diff/260001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:180: DCHECK(decl->scope()->RemoveInnerScope(new_func_scope)); On 2016/11/29 10:48:50, marja wrote: > Ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
some minor comments from my side only, lgtm otherwise on parser / ast / factory https://codereview.chromium.org/2398023002/diff/280001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/ast/scopes.cc#newc... src/ast/scopes.cc:553: (info->asm_function_scope() && scope->scope_type() == FUNCTION_SCOPE)); is_function_scope() https://codereview.chromium.org/2398023002/diff/280001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/factory.cc#newcode... src/factory.cc:2275: Handle<ScopeInfo> scope_info = handle(ScopeInfo::Empty(isolate())); Handle<ScopeInfo> scope_info(ScopeInfo::Empty(isolate()); https://codereview.chromium.org/2398023002/diff/280001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/parsing/parser.cc#... src/parsing/parser.cc:866: DCHECK(factory()->zone() == info->zone()); DCHECK_EQ
I guess Toon & Marja have this covered from the things I could review?
On 2016/11/29 06:30:35, bradn wrote: > PTAL > > +jochen (have I meshed well with what you're adding with function ids?) > +verawest PTAL > > I notice that in some places AstNodeFactory uses the zone out of an > AstValueFactory. Do I need to be concerned I'm still generating the bulk of per > function info into a the outer zone? > > https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-typer.h > File src/asmjs/asm-typer.h (right): > > https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-typer.h#... > src/asmjs/asm-typer.h:262: AsmType* > ValidateModuleBeforeFunctionsPhase(FunctionLiteral* fun); > On 2016/11/28 10:40:11, titzer wrote: > > Seems a little weird to use AsmType* as the return type to indicate > > success/fail. Is it ever used in another way? > > Used so the FAIL macro can be shared throughout. > Not sure that's a great reason. > > https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... > File src/asmjs/asm-wasm-builder.cc (right): > > https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... > src/asmjs/asm-wasm-builder.cc:139: Handle<SharedFunctionInfo> shared = > On 2016/11/28 10:40:11, titzer wrote: > > I'm wondering if we need to squirrel these shared function infos away and save > > them for later use with the module... > > Agreed, will add a TODO. > > https://codereview.chromium.org/2398023002/diff/180001/src/asmjs/asm-wasm-bui... > src/asmjs/asm-wasm-builder.cc:148: > info.set_ast_value_factory(ast_value_factory_); > On 2016/11/28 11:47:21, marja wrote: > > On 2016/11/27 01:14:02, bradn wrote: > > > I'd rather not have to do this, but (see other comment) Scopes seem to rely > on > > a > > > common AstValueFactory being used for variables names. > > > Suggestions welcome. > > > > This is fine (see my other comment about how th fix the Zone mismatch). > > Ok, thanks > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc > File src/ast/scopes.cc (right): > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... > src/ast/scopes.cc:549: // DCHECK(scope->scope_type() == SCRIPT_SCOPE || > On 2016/11/28 10:40:11, titzer wrote: > > Can you explain why this DCHECK is no longer needed? And delete it... > > Added back with new case and comment. > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... > src/ast/scopes.cc:549: // DCHECK(scope->scope_type() == SCRIPT_SCOPE || > On 2016/11/28 11:10:17, marja wrote: > > On 2016/11/28 10:40:11, titzer wrote: > > > Can you explain why this DCHECK is no longer needed? And delete it... > > > > I think these commented-out-DCHECKs are stuff that fails currently and it's > > still unclear why. They are not to be deleted (but might need an update, idk > > yet) > > Done. > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... > src/ast/scopes.cc:554: scope->set_should_eager_compile(); > On 2016/11/28 10:40:11, titzer wrote: > > Fishy. I don't think we want to set eager compile on the inner asm.js > > functions... > > I think this ends up being moot in the asm case, as these scopes never get used > in a normal compile. > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... > src/ast/scopes.cc:1009: // DCHECK_EQ(factory->zone(), zone()); > On 2016/11/28 10:40:11, titzer wrote: > > delete > > Keeping with additional clause. > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... > src/ast/scopes.cc:1739: // DCHECK(info->script_scope()->is_script_scope()); > On 2016/11/28 10:40:11, titzer wrote: > > delete > > Kept instead. > > https://codereview.chromium.org/2398023002/diff/180001/src/ast/scopes.cc#newc... > src/ast/scopes.cc:1739: // DCHECK(info->script_scope()->is_script_scope()); > On 2016/11/28 11:47:21, marja wrote: > > On 2016/11/28 10:40:11, titzer wrote: > > > delete > > > > Ditto, don't delete; but I think this needs to be updated to reflect the > > relevant condition for you. > > > > I guess you don't have a script scope in this case. > > > > info->script_scope()->is_script_scope() || info->asm_function_scope() > >something > > something (or maybe asm_function_scope->outer_scope()->something). > > doing this above. > Actually this one can be kept as is. > > https://codereview.chromium.org/2398023002/diff/180001/src/compiler.h > File src/compiler.h (right): > > https://codereview.chromium.org/2398023002/diff/180001/src/compiler.h#newcode75 > src/compiler.h:75: static Handle<SharedFunctionInfo> > NewSharedFunctionInfoFromLiteral( > On 2016/11/28 10:40:11, titzer wrote: > > I think this probably belongs on Factory. > > Moved the private method there. > > https://codereview.chromium.org/2398023002/diff/180001/src/parsing/parser.cc > File src/parsing/parser.cc (right): > > https://codereview.chromium.org/2398023002/diff/180001/src/parsing/parser.cc#... > src/parsing/parser.cc:858: if (info->asm_function_scope()) { > On 2016/11/28 10:40:11, titzer wrote: > > Also fishy. I am not sure why we have to manually mess with the scope chain > > here. > > Agreed. > I had tried instead to set the outer function on the ParseInfo, > but DeserializeScopeChain assumes that outer scopes are fully resolved in > various way: > * Have slots assigned to them > * Aren't going to be disconnected from the chain > > To be honest I'm a little fuzzy on what deserialization means in this context? We have Scope* and ScopeInfo*, where the latter is a heap-allocated data structure that is a compressed form of a scope. We use the ScopeInfo to recreate an appropriate Scope when compiling an inner function.
lgtm with comments https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-js.cc File src/asmjs/asm-js.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-js.cc#ne... src/asmjs/asm-js.cc:156: v8::internal::wasm::AsmWasmBuilder builder(info->isolate(), info->zone(), Can you drop the v8::internal:: qualifiers? https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-js.cc#ne... src/asmjs/asm-js.cc:159: i::Handle<i::FixedArray> foreign_globals; I think you can drop the i::FixedArray qualification here. https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.cc File src/asmjs/asm-typer.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.cc... src/asmjs/asm-typer.cc:130: bool AsmTyper::SourceLayoutTracker::Section::OverlapsWith( This check is actually stricter than OverlapsWith; it is that this section is actually starts and ends before {other}. Different name? https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.h File src/asmjs/asm-typer.h (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.h#... src/asmjs/asm-typer.h:139: void FirstForwardUseIs(int source_location); rename to SetFirstForwardUse? Can also do that in a followup CL. https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:135: i::Zone zone(isolate_->allocator(), ZONE_NAME); Can you leave off the i:: prefix, since we really only use that in code that mixes up API handles and internal handles, and this should all be internal.
https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-js.cc File src/asmjs/asm-js.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-js.cc#ne... src/asmjs/asm-js.cc:156: v8::internal::wasm::AsmWasmBuilder builder(info->isolate(), info->zone(), On 2016/11/29 14:57:37, titzer wrote: > Can you drop the v8::internal:: qualifiers? Done. https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-js.cc#ne... src/asmjs/asm-js.cc:159: i::Handle<i::FixedArray> foreign_globals; On 2016/11/29 14:57:37, titzer wrote: > I think you can drop the i::FixedArray qualification here. Done. https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.cc File src/asmjs/asm-typer.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.cc... src/asmjs/asm-typer.cc:130: bool AsmTyper::SourceLayoutTracker::Section::OverlapsWith( On 2016/11/29 14:57:37, titzer wrote: > This check is actually stricter than OverlapsWith; it is that this section is > actually starts and ends before {other}. > > Different name? Actually this is too strict. It happens to work because other parts of validation prescribe the ordering, but it's clear to just have this check for overlap. Changed the comparison instead. https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.h File src/asmjs/asm-typer.h (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.h#... src/asmjs/asm-typer.h:139: void FirstForwardUseIs(int source_location); On 2016/11/29 14:57:37, titzer wrote: > rename to SetFirstForwardUse? Can also do that in a followup CL. Done. https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-wasm-bui... File src/asmjs/asm-wasm-builder.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-wasm-bui... src/asmjs/asm-wasm-builder.cc:135: i::Zone zone(isolate_->allocator(), ZONE_NAME); On 2016/11/29 14:57:37, titzer wrote: > Can you leave off the i:: prefix, since we really only use that in code that > mixes up API handles and internal handles, and this should all be internal. Done. https://codereview.chromium.org/2398023002/diff/280001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/ast/scopes.cc#newc... src/ast/scopes.cc:553: (info->asm_function_scope() && scope->scope_type() == FUNCTION_SCOPE)); On 2016/11/29 14:39:59, Toon Verwaest wrote: > is_function_scope() Done. https://codereview.chromium.org/2398023002/diff/280001/src/factory.cc File src/factory.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/factory.cc#newcode... src/factory.cc:2275: Handle<ScopeInfo> scope_info = handle(ScopeInfo::Empty(isolate())); On 2016/11/29 14:39:59, Toon Verwaest wrote: > Handle<ScopeInfo> scope_info(ScopeInfo::Empty(isolate()); Done. https://codereview.chromium.org/2398023002/diff/280001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/parsing/parser.cc#... src/parsing/parser.cc:866: DCHECK(factory()->zone() == info->zone()); On 2016/11/29 14:39:59, Toon Verwaest wrote: > DCHECK_EQ Done.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org, verwaest@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2398023002/#ps300001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.cc File src/asmjs/asm-typer.cc (right): https://codereview.chromium.org/2398023002/diff/280001/src/asmjs/asm-typer.cc... src/asmjs/asm-typer.cc:130: bool AsmTyper::SourceLayoutTracker::Section::OverlapsWith( On 2016/11/29 22:09:06, bradn wrote: > On 2016/11/29 14:57:37, titzer wrote: > > This check is actually stricter than OverlapsWith; it is that this section is > > actually starts and ends before {other}. > > > > Different name? > > Actually this is too strict. > It happens to work because other parts of validation prescribe the ordering, but > it's clear to just have this check for overlap. > Changed the comparison instead. Actually this is not overlap, renaming.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org, marja@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2398023002/#ps320001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1480462256759580,
"parent_rev": "4b57c62fa2d62fa21f5f67a0f0f718e68c36cc07", "commit_rev":
"c1bf6d37db74651945d9727a36741ada52f94d31"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. Add some additional output to a few tests that's helpful to see which stage they fail at. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... ========== to ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. Add some additional output to a few tests that's helpful to see which stage they fail at. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. Add some additional output to a few tests that's helpful to see which stage they fail at. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... ========== to ========== [wasm] asm.js - Parse and convert asm.js to wasm a function at a time. Make the AsmWasmBuilder drive the process of typing and potentially parsing function bodies. This will allow us to keep only a single asm.js function's AST in memory as we convert to WebAssembly. This is needed to keep our memory footprint low. Add some additional output to a few tests that's helpful to see which stage they fail at. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=4203 LOG=N R=marja@chromium.org,adamk@chromium.org,aseemgarg@chromium.org,titzer@chromiu... Committed: https://crrev.com/14e05c104684226ecc2ecaef9794d55803f52023 Cr-Commit-Position: refs/heads/master@{#41372} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/14e05c104684226ecc2ecaef9794d55803f52023 Cr-Commit-Position: refs/heads/master@{#41372} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
