Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2757693003:
[wasm][asm.js] Asm.js -> wasm custom parser. (Closed)
|
Created:
3 years, 9 months ago by bradnelson Modified:
3 years, 8 months ago CC:
v8-reviews_googlegroups.com, Michael Hablich, Karl Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[wasm][asm.js] Asm.js -> wasm custom parser.
Add the --fast-validate-asm option, which directs asm.js code
to a new parser + validator + wasm code generator,
which is then compiled using WebAssembly.
This parser takes advantage of asm.js structure to linearly parse
asm.js code, keeping a scope stack + a few additional tables to track
varibles.
BUG=v8:6090
BUG=v8:4203
R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org,kschimpf@chromium.org
Review-Url: https://codereview.chromium.org/2757693003
Cr-Commit-Position: refs/heads/master@{#44084}
Committed: https://chromium.googlesource.com/v8/v8/+/083a8d7209e4ee3e43bb03ae3bb2633177d4a77c
Patch Set 1 #
Total comments: 122
Patch Set 2 : fix #Patch Set 3 : fix #
Messages
Total messages: 41 (23 generated)
Description was changed from ========== [wasm][asm.js] Asm.js -> wasm custom parser. Add the --fast-validate-asm option, which directs asm.js code to a new parser + validator + wasm code generator, which is then compiled using WebAssembly. This parser takes advantage of asm.js structure to linearly parse asm.js code, keeping a scope stack + a few additional tables to track varibles. BUG=v8:6090 BUG=v8:4203 ========== to ========== [wasm][asm.js] Asm.js -> wasm custom parser. Add the --fast-validate-asm option, which directs asm.js code to a new parser + validator + wasm code generator, which is then compiled using WebAssembly. This parser takes advantage of asm.js structure to linearly parse asm.js code, keeping a scope stack + a few additional tables to track varibles. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org,kschimpf... ==========
I'm sending this out for review in one lump (now that the scanner is landed). I'm open to subdividing the CL further, but will be out tomorrow, so wanted to given mstarzinger and others a chance to start reviewing. I can easily carve off the src/wasm directory changes, as those can land without the rest (if this is helpful to make the review tractable?). Thanks all!
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: This issue passed the CQ dry run.
Some initial comments. The amount of code is indeed daunting. o.o Not sure how to review this... https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:47: #define SKIP(token) \ Would it make sense to have these as functions instead of macros? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:56: #define SKIP(token) \ For naming consistency, SKIP could be called Expect, like in Parser. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:67: DCHECK(GetCurrentStackPosition() >= stack_limit_); \ I like that the stack check is done when recursing, and not when reading a new token, like in Parser. Because this is logically where it should be. Otoh, the RECURSE macro is pretty scary... Would it make sense to have a STACK_CHECK macro which would be the first thing in each function? It's ofc possible to unintentionally omit it, but it's similarly possible to omit RECURSE(v = someFunc()); and just do v = someFunc(); https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:221: ValueType vtype, bool mut, VarInfo* info) { What's "mut"? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:569: if (Peek('{')) { Would it make sense to have Check() like in Parser? if (Check('{'}) { ... } So Check would basically be your Peek + SKIP. It seems there's a lot of Peek + SKIP here. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h File src/asmjs/asm-parser.h (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:86: std::vector<VarInfo> global_var_info_; std containers vs. Zone containers... expanding vectors in Zone are bad for memory, so, it would need some trickery to store stuff in the Zone. Otoh getting rid of the Zone memory is much faster. What's your take on this trade off in the asm js parser? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:173: void Block(); // 6.5.1 Block Naming nit: would it make sense to name these ParseBlock etc. for consistency w/ Parser?
- Given that this CL is >2.5 kloc I probably won't finish the review today. Haven't looked at asm-parser.cc yet. - I don't really understand the integration of the parser into asm.js pipeline. My comments there are mostly mechanical. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc File src/asmjs/asm-js.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc#newcode169 src/asmjs/asm-js.cc:169: wasm::ZoneBuffer* module; [here & line below:] Initialize w/ nullptr? (The code is correct; both are always initialized before being read. But... one has to read ~70 lines to be sure about that, which IMHO isn't super readable.) https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h File src/asmjs/asm-parser.h (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:33: Zone* zone_; The style guide asks to "generally prefer grouping similar kinds of declarations together" (i.e., methods, then member variables, etc.) It does allow for exceptions ("generally"), and here they're mixed quite wildly. Not sure if this helps readability... :-| https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:75: uint32_t index; Not sure if worthwhile: On 64b platforms this member ordering is somewhat wasteful due to alignment rules. I'd expect there to be fair amount of VarInfo's in one parse, so it might be worthwile to reorder this to avoid 'holes' in the struct. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:90: void DeclareGlobal(VarInfo* info, bool mut, AsmType* type, ValueType vtype, mut? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:133: void BareBegin(BlockKind kind = BlockKind::kOther, What's a Bare (as in: BareBegin / BareEnd)? I take it it's a syntactic construct, but I can't figure out which one. https://codereview.chromium.org/2757693003/diff/1/src/wasm/wasm-module-builde... File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/wasm/wasm-module-builde... src/wasm/wasm-module-builder.cc:188: void WasmFunctionBuilder::StashCode(std::vector<byte>* dst, size_t position) { This method would probably benefit from hosting the dst check. Something like: if (dst == nullptr) { body_.resize(position); return; } size_t len = ... dst->resize(..) memcpy(..) body_.resize(..) [Assuming .resize(.) to current size is a cheap no-op.] https://codereview.chromium.org/2757693003/diff/1/src/wasm/wasm-module-builde... src/wasm/wasm-module-builder.cc:191: dst->resize(body_.size() - position); dst->resize(len) ?
There's only a handful of tests, and I guess this relies strongly on the existing unit tests. Given the complexity of the whole CL, it might be rather useful to have tests that parse w/ the old & new pipeline and compare the results. Even better would be a 'correctness fuzzer' for old + new parser. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:56: #define SKIP(token) \ +1 to Marja's comment. (I don't care so much about consistency; but I wouldn't expect "skip" to make a check.)
https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:28: #define ASSTRING(x) ASSTRING1(x) What does this indirect definition accomplish? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:173: #define FAILURE_RETURN Hrmpf. This (and lines 1249/1259 and 2214/2215) effectively re-defines the FAIL + RECURSE macros for the subsequent text. That's.... surprising, and at the very least deserves a comment here - and elsewhere where this happens - and up there where FAIL + RECURSE are defined, so the reader has a chance to understand this logic. Defining different variants of FAIL + RECURSE does seem like a more understandable option! https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1186: GatherCases(&cases); // Skips { implicitly. Is the comment correct? GatherCases stores GetPosition & later seeks to it. I'd expect that to skip nothing. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1297: } else if (value <= 0x100000000) { < instead <= ??? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:2331: if (Peek('-')) { This whole thing - in addition to being generally weird - duplicates logic from ValidateCase. Maybe we could split up ValidateCase into validating the 'case ..:' bit and validating the statement block separately. Then this could just validate & record the case and skip the block based on bracket counting, while ValidateCase would call the case.. bit and then recurse into the statement.
More comments, now reading through this more carefully. (But I'm only at line 600-ish in the parser.cc.) Can you make sure you have an asmjs-side reviewer who reviews the correctness of this? They probably have better context than we parser people. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc File src/asmjs/asm-js.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc#newcode170 src/asmjs/asm-js.cc:170: wasm::ZoneBuffer* asm_offsets; I think these C-ish declarations can be just moved down to where they're really needed. No need to initialize w/ nullptr. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:221: ValueType vtype, bool mut, VarInfo* info) { On 2017/03/17 13:49:04, marja wrote: > What's "mut"? Nevermind, it's actually "mut" in the spec too, so fine. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:325: RECURSE(ValidateModuleParameters()); Do we actually need to use RECURSE in constructs like this, which cannot recurse arbitrarily deeply? I'd only add it for constructs where it's possible for the user to cause too-deep recursion (like 0 + 0 + 0 + 0 ... (in the non-asmjs parser)). https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:406: void AsmJsParser::ValidateModuleVar(bool mut) { Lost... which part of the spec do I need to read to understand this? 6.1 ValidateModule just recurses to ValidateFuntion, ValidateFunctionTable and ValidateExport. And there is a link to "global declarations" http://asmjs.org/spec/latest/#global-declarations which doesn't lead to anything... Ah, answering to myself, it's "5.5 Global Variable Type Annotations", pls add a comment. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:415: if (scanner_.IsDouble()) { One thing I'm confused about is that which part processes type annotations like |0. Scanner? Parser? Where? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:419: } else if (scanner_.IsUnsigned()) { I wonder whether it would make sense to have one func for getting an unsigned value (if it's unsigned). Like scanner_.IsUnsigned(&value). I guess similar constructs are hot enough that 1 func call versus 2 func calls matters (maybe not here but in other places). (Does that show up in your tests?) https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:525: } This function is quite long, too. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:927: } This function is almost 300 lines; would it be possible to split it?
https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:481: if (!scanner_.IsUnsigned() || scanner_.AsUnsigned() != 0) { Checking for "|0" sounds such a common use case that it would make sense to have a helper for it. Btw why is the error message so different in here in other cases? In other cases, the message is like "bad type annotation", this one doesn't even mention type annotation. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:617: FAIL("Unused function table"); Not sure when this error occurs or what it means. Does it occur when we haven't find any reference to the function table in the functions? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:631: if (count >= table_info->mask + 1) { It's non-trivial to see where the mask is coming from and that storing it has happened at this point in time. Can you DCHECK that mask has a meaningful value? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:738: scanner_.EnterGlobalScope(); This is quite confusing construct! The reason seems to be that when we Skip a token, Scanner already reads in the next one, and it's important that we are in the right scope at that moment. That is a bit scary (unintuitive / fragile). :( And it seems to be fundamentally so. Would it help to have a helper function in Scanner which would hide that complexity? Though, that doesn't fix the fundamental problem (that we need to be in the right scope at an unintuitive point in time). But maybe it's still worth it, seeing that EnterLocalScope is followed by EnterGlobalScope right after. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:866: scanner_.EnterLocalScope(); Ditto... except that this is even more confusing and harder to fix :( https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:957: bool wrap_block = pending_label_ != 0; What's "wrap_block"? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:977: scanner_.Rewind(); Does this construct make labels appear in the local variable table? If yes, why is that ok? (I'm confused!)
https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc File src/asmjs/asm-js.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc#newcode170 src/asmjs/asm-js.cc:170: wasm::ZoneBuffer* asm_offsets; On 2017/03/20 14:28:47, marja wrote: > I think these C-ish declarations can be just moved down to where they're really > needed. No need to initialize w/ nullptr. Ahh, disregard this comment, I missed the fact that they're initialized in an if-else.
Some more comments, but I'm still kinda overwhelmed by the amount of code (I've read through 1000 lines and skimmed 500 and I'm sort of out of energy.) There are some discussions going on about who should review this and on what level; let's see how that evolves. Esp. who will be the reviewer who really understands this and will be able to maintain this. (My comments are mostly generic comments but I have zero knowledge on wasm internals.) https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:313: if (!loop && it->kind != BlockKind::kRegular) { I don't get this function... (esp. its usage, when will "bool loop" be false and when will it be true). https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:663: if (function_info->kind == VarKind::kUnused) { So redeclarations are ok? Which test tests that? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:665: function_info->function_builder = builder_->AddFunction(); This line got me confused for a moment; would it help to rename builder_ to module_builder (I somehow thought builder_ is some kind of function builder here, but it's not). https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:677: scanner_.EnterLocalScope(); Continuing my previous comments about EnterLocalScope; afaict, Parser always knows in which scope we should be, and disambiguates identifiers based on that. So would it be feasible to get rid of the "local scope" / "global scope" in the Scanner, and instead do: scanner_.GetLocal(); or scanner_.GetGlobal(); at the right places? Ofc, Scanner couldn't be able to put the identifier in the local / global table before one of these get called (so, it couldn't do it in advance). Another confusing construct is: scanner_.EnterLocalScope(); if (!scanner_.IsLocal()) { ... } << this looks like it's checking "is it local (as opposed to global)" which it's not; it's really "IsIdentifier". https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1020: if (ret->IsA(AsmType::Double())) { This kind of check (checking the type and setting return_type_ based on it) is probably so common that it could go to a helper func. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1053: Loop(pending_label_); I don't understand (based on looking at this for a short time but not going deeper) what Begin and Loop do. Probably because I don't know how loops are expressed in wasm. Maybe add a comment (at the function definition?). https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1059: current_function_builder_->Emit(kExprI32Eqz); Comment pls. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1081: current_function_builder_->Emit(kExprI32Eqz); Ditto. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1126: if (scanner_.IsGlobal() || scanner_.IsLocal()) { Why can it be Global here? Can this construct appear outside a function? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1148: depth = FindLabelDepth(0, true); Why is the "bool loop" true here and false in BreakStatement? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1153: current_function_builder_->Emit(kExprBr); This sounds suspicious. BreakStatement and ContinueStatement emit the same? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1225: if ((negate && scanner_.AsUnsigned() > 0x80000000) || This logic is duplicated in several places -> can you put this to a helper func? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1311: call_coercion_ = nullptr; What's call_coercion_? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1408: } else if (scanner_.IsLocal() || scanner_.IsGlobal()) { For cases like this, if the Scanner no longer knows whether we're in the local or global scope, then the Parser must know and store it. But it fits better in the parser! https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h File src/asmjs/asm-parser.h (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:143: AsmType* call_coercion_; Pls add a comment about what this is?
LGTM modulo comments. I agree with Marja's comment. Given the size of this CL it seems infeasible to do a deep-dive look of all the individual grammar rules. That would consume a lot of reviewer time. It might make sense to expedite getting it into the code base, get test coverage early and polish it incrementally. Re: "who will be the reviewer who really understands this and will be able to maintain this", I am nowhere close to really understand this. But I am willing to take on some of the (especially short-term) maintenance burden that results as a fallout of this. Marja, Daniel, thanks a lot for all the time you invested in doing a deep dive into this and the super valuable comments. Very much appreciated! https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:7: // Required to get M_E etc. in MSVC. nit: Could we make this comment more explicit, that it is about the uses in STDLIB_MATH_VALUE_LIST, it took me a while to find the uses. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:173: #define FAILURE_RETURN On 2017/03/17 18:01:29, vogelheim wrote: > Hrmpf. This (and lines 1249/1259 and 2214/2215) effectively re-defines the FAIL > + RECURSE macros for the subsequent text. That's.... surprising, and at the very > least deserves a comment here - and elsewhere where this happens - and up there > where FAIL + RECURSE are defined, so the reader has a chance to understand this > logic. > > Defining different variants of FAIL + RECURSE does seem like a more > understandable option! +1, I would also prefer to just define two different macros instead of re-configuring the existing FAIL macro. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h File src/asmjs/asm-parser.h (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:22: class AsmJsParser { nit: Can we add a high-level description of what the intent of this class is. I was thinking about mentioning that it performs parsing, asm.js validation and translation to WASM in (almost) a single pass. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:33: Zone* zone_; On 2017/03/17 17:27:06, vogelheim wrote: > The style guide asks to "generally prefer grouping similar kinds of declarations > together" (i.e., methods, then member variables, etc.) It does allow for > exceptions ("generally"), and here they're mixed quite wildly. Not sure if this > helps readability... :-| +1. Lets group similar declaration together, all fields at the bottom, all simple accessors together. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:106: std::list<GlobalImport> global_imports_; nit: Could we use ZoneLinkedList here? https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:129: std::vector<BlockInfo> block_stack_; This really seems to be used as a stack, would it make sense to use a {ZoneStack} instead? WDYT? https://codereview.chromium.org/2757693003/diff/1/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2757693003/diff/1/src/flag-definitions.h#newc... src/flag-definitions.h:553: DEFINE_IMPLICATION(fast_validate_asm, validate_asm) Do we really need this implication? IMHO the --fast-validate-asm flag shouldn't be in the code-base for all too long. I would like to make it the default soon. In the meantime one would just need to pass both flags explicitly to trigger the new parser. WDYT? https://codereview.chromium.org/2757693003/diff/1/test/mjsunit/wasm/asm-wasm.js File test/mjsunit/wasm/asm-wasm.js (right): https://codereview.chromium.org/2757693003/diff/1/test/mjsunit/wasm/asm-wasm.... test/mjsunit/wasm/asm-wasm.js:1150: // TODO(bradnelson): Fix handling of this case and re-enable. nit: You could reference issue 6127 here. https://codereview.chromium.org/2757693003/diff/1/test/mjsunit/wasm/asm-wasm.... test/mjsunit/wasm/asm-wasm.js:1190: // TODO(bradnelson): Fix handling of this case and re-enable. nit: You could reference issue 6127 here.
kschimpf@google.com changed reviewers: + kschimpf@google.com
LGTM too. Like mentioned above, once the existing comments go in, I would be willing to let this CL be committed. I'm also willing to help sort things out if problems arise from the code being committed.
lgtm based on the offline discussion
https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:316: if (label == 0 || it->label == label) { Documenting offline disucssion: IMO this looks like it'll find the wrong block in this case: some loop { if (..) { if (..) { << will find this block?? break; } } }
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...
Thanks for all the hard work reviewing. Kicking off a try now and will land assuming it's ok. Sorry again for the size and hairiness! https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc File src/asmjs/asm-js.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc#newcode169 src/asmjs/asm-js.cc:169: wasm::ZoneBuffer* module; On 2017/03/17 17:27:06, vogelheim wrote: > [here & line below:] Initialize w/ nullptr? > > (The code is correct; both are always initialized before being read. But... one > has to read ~70 lines to be sure about that, which IMHO isn't super readable.) Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc#newcode170 src/asmjs/asm-js.cc:170: wasm::ZoneBuffer* asm_offsets; On 2017/03/20 14:28:47, marja wrote: > I think these C-ish declarations can be just moved down to where they're really > needed. No need to initialize w/ nullptr. Acknowledged. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-js.cc#newcode170 src/asmjs/asm-js.cc:170: wasm::ZoneBuffer* asm_offsets; On 2017/03/20 15:49:23, marja wrote: > On 2017/03/20 14:28:47, marja wrote: > > I think these C-ish declarations can be just moved down to where they're > really > > needed. No need to initialize w/ nullptr. > > Ahh, disregard this comment, I missed the fact that they're initialized in an > if-else. Acknowledged. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc File src/asmjs/asm-parser.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:7: // Required to get M_E etc. in MSVC. On 2017/03/22 14:55:51, Michael Starzinger wrote: > nit: Could we make this comment more explicit, that it is about the uses in > STDLIB_MATH_VALUE_LIST, it took me a while to find the uses. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:28: #define ASSTRING(x) ASSTRING1(x) On 2017/03/17 18:01:29, vogelheim wrote: > What does this indirect definition accomplish? Sorry leftover, inlined. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:47: #define SKIP(token) \ On 2017/03/17 13:49:04, marja wrote: > Would it make sense to have these as functions instead of macros? It might be possible (and might be denser and thereby fast) if we made everything able to tolerate failed state up until the next check. I'm going to leave it for the moment, as I'd be concerned I might subtly break things without care. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:56: #define SKIP(token) \ On 2017/03/17 13:49:04, marja wrote: > For naming consistency, SKIP could be called Expect, like in Parser. As I'm unable to switch this from being a macro at the moment (as it calls FAIL), and EXPECT would clash with testing macros, I'm gonna go with EXPECT_TOKEN https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:56: #define SKIP(token) \ On 2017/03/17 17:31:13, vogelheim wrote: > +1 to Marja's comment. > > (I don't care so much about consistency; but I wouldn't expect "skip" to make a > check.) Acknowledged. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:67: DCHECK(GetCurrentStackPosition() >= stack_limit_); \ On 2017/03/17 13:49:04, marja wrote: > I like that the stack check is done when recursing, and not when reading a new > token, like in Parser. Because this is logically where it should be. > > Otoh, the RECURSE macro is pretty scary... > > Would it make sense to have a STACK_CHECK macro which would be the first thing > in each function? It's ofc possible to unintentionally omit it, but it's > similarly possible to omit RECURSE(v = someFunc()); and just do v = someFunc(); Not keen on this pattern myself (got it from the ast-visitor family). The trouble with moving it to the beginning of each function is that on failure callers won't know to back all the way out (i.e. you called a thing that overflowed, but keep going). https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:173: #define FAILURE_RETURN On 2017/03/17 18:01:29, vogelheim wrote: > Hrmpf. This (and lines 1249/1259 and 2214/2215) effectively re-defines the FAIL > + RECURSE macros for the subsequent text. That's.... surprising, and at the very > least deserves a comment here - and elsewhere where this happens - and up there > where FAIL + RECURSE are defined, so the reader has a chance to understand this > logic. > > Defining different variants of FAIL + RECURSE does seem like a more > understandable option! Also applies to SKIP->EXPECT_TOKEN. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:173: #define FAILURE_RETURN On 2017/03/22 14:55:51, Michael Starzinger wrote: > On 2017/03/17 18:01:29, vogelheim wrote: > > Hrmpf. This (and lines 1249/1259 and 2214/2215) effectively re-defines the > FAIL > > + RECURSE macros for the subsequent text. That's.... surprising, and at the > very > > least deserves a comment here - and elsewhere where this happens - and up > there > > where FAIL + RECURSE are defined, so the reader has a chance to understand > this > > logic. > > > > Defining different variants of FAIL + RECURSE does seem like a more > > understandable option! > > +1, I would also prefer to just define two different macros instead of > re-configuring the existing FAIL macro. Acknowledged. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:221: ValueType vtype, bool mut, VarInfo* info) { On 2017/03/17 13:49:04, marja wrote: > What's "mut"? mutable https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:221: ValueType vtype, bool mut, VarInfo* info) { On 2017/03/20 14:28:47, marja wrote: > On 2017/03/17 13:49:04, marja wrote: > > What's "mut"? > > Nevermind, it's actually "mut" in the spec too, so fine. Yeah that was the other reason I used it, but changed to mutable_variable for general readability. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:313: if (!loop && it->kind != BlockKind::kRegular) { On 2017/03/21 13:31:44, marja wrote: > I don't get this function... (esp. its usage, when will "bool loop" be false and > when will it be true). Broke this apart into two methods for finding breaks or continues. Hopefully that will be clearer. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:316: if (label == 0 || it->label == label) { On 2017/03/23 12:16:55, marja wrote: > Documenting offline disucssion: IMO this looks like it'll find the wrong block > in this case: > > some loop { > if (..) { > if (..) { << will find this block?? > break; > } > } > } > So I believe this works. The 'if' type blocks are kOther, whereas appropriate ones are kRegular / kLoop. I've refactored hopefully making it clearer, but the stack's type names are still not to my satisfaction. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:325: RECURSE(ValidateModuleParameters()); On 2017/03/20 14:28:47, marja wrote: > Do we actually need to use RECURSE in constructs like this, which cannot recurse > arbitrarily deeply? > > I'd only add it for constructs where it's possible for the user to cause > too-deep recursion (like 0 + 0 + 0 + 0 ... (in the non-asmjs parser)). RECURSE doubles as both a stack check and an early bail out if the called method fails. I think this one is currently needed in case the module parameters are invalid. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:406: void AsmJsParser::ValidateModuleVar(bool mut) { On 2017/03/20 14:28:47, marja wrote: > Lost... which part of the spec do I need to read to understand this? 6.1 > ValidateModule just recurses to ValidateFuntion, ValidateFunctionTable and > ValidateExport. And there is a link to "global declarations" > http://asmjs.org/spec/latest/#global-declarations which doesn't lead to > anything... > > Ah, answering to myself, it's "5.5 Global Variable Type Annotations", pls add a > comment. Acknowledged. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:415: if (scanner_.IsDouble()) { On 2017/03/20 14:28:47, marja wrote: > One thing I'm confused about is that which part processes type annotations like > |0. Scanner? Parser? Where? So all of that lives in the parser. It's not shared because it's used in slightly different contexts (did make a helper for the 0). It's used for: * Return type annotations * Callsite return type annotations * Callsite parameter type annotations * parameter type annotations * Global imports of integers. There's also a couple places were peeking ahead is used to know if things are a type annotation. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:419: } else if (scanner_.IsUnsigned()) { On 2017/03/20 14:28:47, marja wrote: > I wonder whether it would make sense to have one func for getting an unsigned > value (if it's unsigned). Like scanner_.IsUnsigned(&value). > > I guess similar constructs are hot enough that 1 func call versus 2 func calls > matters (maybe not here but in other places). (Does that show up in your tests?) Added something like this in parser for now. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:481: if (!scanner_.IsUnsigned() || scanner_.AsUnsigned() != 0) { On 2017/03/20 14:57:57, marja wrote: > Checking for "|0" sounds such a common use case that it would make sense to have > a helper for it. > > Btw why is the error message so different in here in other cases? In other > cases, the message is like "bad type annotation", this one doesn't even mention > type annotation. Added a helper for checking for 0 which fit 3 places. Improved message. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:525: } On 2017/03/20 14:28:47, marja wrote: > This function is quite long, too. Chopped it up into a few parts. In general this isn't as decomposed as would be ideal. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:569: if (Peek('{')) { On 2017/03/17 13:49:04, marja wrote: > Would it make sense to have Check() like in Parser? > > if (Check('{'}) { ... } > > So Check would basically be your Peek + SKIP. It seems there's a lot of Peek + > SKIP here. Great idea. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:617: FAIL("Unused function table"); On 2017/03/20 14:57:57, marja wrote: > Not sure when this error occurs or what it means. Does it occur when we haven't > find any reference to the function table in the functions? Meant a function table was defined but never used. However, while unlikely, on a closer look at the spec, I don't see anything that would ban this. Changed to ignore it instead. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:631: if (count >= table_info->mask + 1) { On 2017/03/20 14:57:58, marja wrote: > It's non-trivial to see where the mask is coming from and that storing it has > happened at this point in time. Can you DCHECK that mask has a meaningful value? Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:663: if (function_info->kind == VarKind::kUnused) { On 2017/03/21 13:31:43, marja wrote: > So redeclarations are ok? Which test tests that? Ooh, messed that up. They're allowed to be defined out of order but not redefined. Fixed. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:665: function_info->function_builder = builder_->AddFunction(); On 2017/03/21 13:31:44, marja wrote: > This line got me confused for a moment; would it help to rename builder_ to > module_builder (I somehow thought builder_ is some kind of function builder > here, but it's not). Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:677: scanner_.EnterLocalScope(); On 2017/03/21 13:31:44, marja wrote: > Continuing my previous comments about EnterLocalScope; afaict, Parser always > knows in which scope we should be, and disambiguates identifiers based on that. > So would it be feasible to get rid of the "local scope" / "global scope" in the > Scanner, and instead do: > > scanner_.GetLocal(); or > scanner_.GetGlobal(); > > at the right places? > > Ofc, Scanner couldn't be able to put the identifier in the local / global table > before one of these get called (so, it couldn't do it in advance). > > Another confusing construct is: > > scanner_.EnterLocalScope(); > if (!scanner_.IsLocal()) { ... } << this looks like it's checking "is it local > (as opposed to global)" which it's not; it's really "IsIdentifier". Agreed this is confusing and needs to be redone. Added a TODO. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:738: scanner_.EnterGlobalScope(); On 2017/03/20 14:57:58, marja wrote: > This is quite confusing construct! > > The reason seems to be that when we Skip a token, Scanner already reads in the > next one, and it's important that we are in the right scope at that moment. That > is a bit scary (unintuitive / fragile). :( And it seems to be fundamentally so. > > Would it help to have a helper function in Scanner which would hide that > complexity? Though, that doesn't fix the fundamental problem (that we need to be > in the right scope at an unintuitive point in time). > > But maybe it's still worth it, seeing that EnterLocalScope is followed by > EnterGlobalScope right after. Agreed this is very messy. I had thought about deferring picking local/global until the context is clear. Added a TODO as this might be subtle. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:866: scanner_.EnterLocalScope(); On 2017/03/20 14:57:57, marja wrote: > Ditto... except that this is even more confusing and harder to fix :( Same as above. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:927: } On 2017/03/20 14:28:47, marja wrote: > This function is almost 300 lines; would it be possible to split it? Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:957: bool wrap_block = pending_label_ != 0; On 2017/03/20 14:57:57, marja wrote: > What's "wrap_block"? Renamed can_break_to_block i.e. this is deciding if emitting a real wasm block for this JS block is needed. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:977: scanner_.Rewind(); On 2017/03/20 14:57:57, marja wrote: > Does this construct make labels appear in the local variable table? If yes, why > is that ok? (I'm confused!) Added a comment. Surprisingly asm.js code in the wide actually has labels that match names of locals or globals. I could mark them separately, but since labels are only ever resolved from the block_stack I'm able to get away with not having a separate category for them. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1020: if (ret->IsA(AsmType::Double())) { On 2017/03/21 13:31:44, marja wrote: > This kind of check (checking the type and setting return_type_ based on it) is > probably so common that it could go to a helper func. A bunch of these are close, but no patterned jumped out. I'll leave that for after it lands. Added a TODO. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1053: Loop(pending_label_); On 2017/03/21 13:31:43, marja wrote: > I don't understand (based on looking at this for a short time but not going > deeper) what Begin and Loop do. Probably because I don't know how loops are > expressed in wasm. Maybe add a comment (at the function definition?). Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1059: current_function_builder_->Emit(kExprI32Eqz); On 2017/03/21 13:31:43, marja wrote: > Comment pls. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1081: current_function_builder_->Emit(kExprI32Eqz); On 2017/03/21 13:31:43, marja wrote: > Ditto. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1126: if (scanner_.IsGlobal() || scanner_.IsLocal()) { On 2017/03/21 13:31:43, marja wrote: > Why can it be Global here? Can this construct appear outside a function? Currently reusing global / local identifiers for labels. Added a comment. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1148: depth = FindLabelDepth(0, true); On 2017/03/21 13:31:44, marja wrote: > Why is the "bool loop" true here and false in BreakStatement? Refactored for clarity, they share the same stack, but look for different things. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1153: current_function_builder_->Emit(kExprBr); On 2017/03/21 13:31:43, marja wrote: > This sounds suspicious. BreakStatement and ContinueStatement emit the same? Br is the only unconditional construct in wasm. It gets used to break both out blocks and loops :-/ https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1186: GatherCases(&cases); // Skips { implicitly. On 2017/03/17 18:01:29, vogelheim wrote: > Is the comment correct? GatherCases stores GetPosition & later seeks to it. I'd > expect that to skip nothing. The comment is correct, that is a little odd though. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1225: if ((negate && scanner_.AsUnsigned() > 0x80000000) || On 2017/03/21 13:31:43, marja wrote: > This logic is duplicated in several places -> can you put this to a helper func? Should do. Don't see an easy pattern. Added a TODO. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1297: } else if (value <= 0x100000000) { On 2017/03/17 18:01:28, vogelheim wrote: > < instead <= ??? Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1311: call_coercion_ = nullptr; On 2017/03/21 13:31:43, marja wrote: > What's call_coercion_? Added comment in header. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:1408: } else if (scanner_.IsLocal() || scanner_.IsGlobal()) { On 2017/03/21 13:31:43, marja wrote: > For cases like this, if the Scanner no longer knows whether we're in the local > or global scope, then the Parser must know and store it. But it fits better in > the parser! So this was a particularly tricky spot. The || was to share identical code for both paths. There's actually 3 cases, these can either local/global variables, or an undefined global, which has better be used as a label. Added a DCHECK of this. Probably could use some refactoring though. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.cc#new... src/asmjs/asm-parser.cc:2331: if (Peek('-')) { On 2017/03/17 18:01:29, vogelheim wrote: > This whole thing - in addition to being generally weird - duplicates logic from > ValidateCase. > > Maybe we could split up ValidateCase into validating the 'case ..:' bit and > validating the statement block separately. Then this could just validate & > record the case and skip the block based on bracket counting, while ValidateCase > would call the case.. bit and then recurse into the statement. Not quite following your meaning. This probably doesn't need to check the ranges (as the other one checks them). It just felt weird to cast without the check. But as that costs some perf and should be fine, dropping it. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h File src/asmjs/asm-parser.h (right): https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:22: class AsmJsParser { On 2017/03/22 14:55:51, Michael Starzinger wrote: > nit: Can we add a high-level description of what the intent of this class is. I > was thinking about mentioning that it performs parsing, asm.js validation and > translation to WASM in (almost) a single pass. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:33: Zone* zone_; On 2017/03/22 14:55:51, Michael Starzinger wrote: > On 2017/03/17 17:27:06, vogelheim wrote: > > The style guide asks to "generally prefer grouping similar kinds of > declarations > > together" (i.e., methods, then member variables, etc.) It does allow for > > exceptions ("generally"), and here they're mixed quite wildly. Not sure if > this > > helps readability... :-| > > +1. Lets group similar declaration together, all fields at the bottom, all > simple accessors together. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:33: Zone* zone_; On 2017/03/17 17:27:06, vogelheim wrote: > The style guide asks to "generally prefer grouping similar kinds of declarations > together" (i.e., methods, then member variables, etc.) It does allow for > exceptions ("generally"), and here they're mixed quite wildly. Not sure if this > helps readability... :-| Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:75: uint32_t index; On 2017/03/17 17:27:07, vogelheim wrote: > Not sure if worthwhile: > > On 64b platforms this member ordering is somewhat wasteful due to alignment > rules. I'd expect there to be fair amount of VarInfo's in one parse, so it might > be worthwile to reorder this to avoid 'holes' in the struct. Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:86: std::vector<VarInfo> global_var_info_; On 2017/03/17 13:49:04, marja wrote: > std containers vs. Zone containers... expanding vectors in Zone are bad for > memory, so, it would need some trickery to store stuff in the Zone. Otoh getting > rid of the Zone memory is much faster. What's your take on this trade off in the > asm js parser? I suspect switching to Zone allocation might be a win for the locals, but might also require tuning to figure out a good start size. For the globals, they stick around for the whole time, so probably is moot for them. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:90: void DeclareGlobal(VarInfo* info, bool mut, AsmType* type, ValueType vtype, On 2017/03/17 17:27:06, vogelheim wrote: > mut? mutable, but clashes with the c++ keyword. changed to mutable_variable. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:106: std::list<GlobalImport> global_imports_; On 2017/03/22 14:55:51, Michael Starzinger wrote: > nit: Could we use ZoneLinkedList here? Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:129: std::vector<BlockInfo> block_stack_; On 2017/03/22 14:55:51, Michael Starzinger wrote: > This really seems to be used as a stack, would it make sense to use a > {ZoneStack} instead? WDYT? It unfortunately walks the stack to scan for labels (without unwinding it). https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:133: void BareBegin(BlockKind kind = BlockKind::kOther, On 2017/03/17 17:27:06, vogelheim wrote: > What's a Bare (as in: BareBegin / BareEnd)? I take it it's a syntactic > construct, but I can't figure out which one. Not happy with these names myself. The Bare* ones just add block_stack_ layers, Begin/Loop/End actually emit wasm blocks/loops. Part of the naming issue is too much "block": the wasm opcode block, the general category that loop + block belong to, the javascript block and the method to parse it below. Added a comment, but welcome suggestions. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:143: AsmType* call_coercion_; On 2017/03/21 13:31:44, marja wrote: > Pls add a comment about what this is? Done. https://codereview.chromium.org/2757693003/diff/1/src/asmjs/asm-parser.h#newc... src/asmjs/asm-parser.h:173: void Block(); // 6.5.1 Block On 2017/03/17 13:49:04, marja wrote: > Naming nit: would it make sense to name these ParseBlock etc. for consistency w/ > Parser? Unsure, had named them this way to mirror the asm.js spec. https://codereview.chromium.org/2757693003/diff/1/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2757693003/diff/1/src/flag-definitions.h#newc... src/flag-definitions.h:553: DEFINE_IMPLICATION(fast_validate_asm, validate_asm) On 2017/03/22 14:55:51, Michael Starzinger wrote: > Do we really need this implication? IMHO the --fast-validate-asm flag shouldn't > be in the code-base for all too long. I would like to make it the default soon. > In the meantime one would just need to pass both flags explicitly to trigger the > new parser. WDYT? done https://codereview.chromium.org/2757693003/diff/1/src/wasm/wasm-module-builde... File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2757693003/diff/1/src/wasm/wasm-module-builde... src/wasm/wasm-module-builder.cc:188: void WasmFunctionBuilder::StashCode(std::vector<byte>* dst, size_t position) { On 2017/03/17 17:27:07, vogelheim wrote: > This method would probably benefit from hosting the dst check. Something like: > > if (dst == nullptr) { > body_.resize(position); > return; > } > > size_t len = ... > dst->resize(..) > memcpy(..) > body_.resize(..) > > > [Assuming .resize(.) to current size is a cheap no-op.] Done. https://codereview.chromium.org/2757693003/diff/1/src/wasm/wasm-module-builde... src/wasm/wasm-module-builder.cc:191: dst->resize(body_.size() - position); On 2017/03/17 17:27:07, vogelheim wrote: > dst->resize(len) ? Done. https://codereview.chromium.org/2757693003/diff/1/test/mjsunit/wasm/asm-wasm.js File test/mjsunit/wasm/asm-wasm.js (right): https://codereview.chromium.org/2757693003/diff/1/test/mjsunit/wasm/asm-wasm.... test/mjsunit/wasm/asm-wasm.js:1150: // TODO(bradnelson): Fix handling of this case and re-enable. On 2017/03/22 14:55:52, Michael Starzinger wrote: > nit: You could reference issue 6127 here. Done. https://codereview.chromium.org/2757693003/diff/1/test/mjsunit/wasm/asm-wasm.... test/mjsunit/wasm/asm-wasm.js:1190: // TODO(bradnelson): Fix handling of this case and re-enable. On 2017/03/22 14:55:52, Michael Starzinger wrote: > nit: You could reference issue 6127 here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
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...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23196) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/23109) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
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: This issue passed the CQ dry run.
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kschimpf@google.com, mstarzinger@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/2757693003/#ps40001 (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": 40001, "attempt_start_ts": 1490334745965430,
"parent_rev": "bdf32cf1bc96982ff5a22195d874617ffae03e79", "commit_rev":
"083a8d7209e4ee3e43bb03ae3bb2633177d4a77c"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Asm.js -> wasm custom parser. Add the --fast-validate-asm option, which directs asm.js code to a new parser + validator + wasm code generator, which is then compiled using WebAssembly. This parser takes advantage of asm.js structure to linearly parse asm.js code, keeping a scope stack + a few additional tables to track varibles. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org,kschimpf... ========== to ========== [wasm][asm.js] Asm.js -> wasm custom parser. Add the --fast-validate-asm option, which directs asm.js code to a new parser + validator + wasm code generator, which is then compiled using WebAssembly. This parser takes advantage of asm.js structure to linearly parse asm.js code, keeping a scope stack + a few additional tables to track varibles. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org,kschimpf... Review-Url: https://codereview.chromium.org/2757693003 Cr-Commit-Position: refs/heads/master@{#44084} Committed: https://chromium.googlesource.com/v8/v8/+/083a8d7209e4ee3e43bb03ae3bb2633177d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/083a8d7209e4ee3e43bb03ae3bb2633177d...
Message was sent while issue was closed.
On 2017/03/24 05:54:04, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/v8/v8/+/083a8d7209e4ee3e43bb03ae3bb2633177d... This caused a size alert. PTAL: https://bugs.chromium.org/p/chromium/issues/detail?id=705417
Message was sent while issue was closed.
On 2017/04/10 17:09:39, agrieve wrote: > On 2017/03/24 05:54:04, commit-bot: I haz the power wrote: > > Committed patchset #3 (id:40001) as > > > https://chromium.googlesource.com/v8/v8/+/083a8d7209e4ee3e43bb03ae3bb2633177d... > > This caused a size alert. PTAL: > https://bugs.chromium.org/p/chromium/issues/detail?id=705417 Could someone that reviewed this change maybe take a look at the size impact: https://bugs.chromium.org/p/chromium/issues/detail?id=705417 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
