This will reduce the permutations of execution engines that we need to maintain.
<rdar://problem/11721515>
Created attachment 153623 [details] work in progress1 Archiving work in progress. There's lots of ugly code, and temporary edits while I'm still figuring things out. Will clean up and refactor when things get closer to the final form.
Created attachment 153659 [details] work in progress 2: added support for switch statement style dispatcher in C loop + some clean up. Builds with ARCHS=i386 but still crashes. x86_64 won't build yet.
Created attachment 156435 [details] Work in progress 3: the 32bit version now runs and passes the JSC tests, v8 bench, and sunspider. 64bit version still needs to be worked on.
Created attachment 157459 [details] Work in progress 4: added 64-bit support and some clean up. Passes JSC tests, v8 bench, and sunspider. On to layout tests next.
Comment on attachment 157459 [details] Work in progress 4: added 64-bit support and some clean up. Passes JSC tests, v8 bench, and sunspider. On to layout tests next. View in context: https://bugs.webkit.org/attachment.cgi?id=157459&action=review Below are some comments so far. > Source/JavaScriptCore/interpreter/VMInspector.h:11 > +class VMInspector { I like this, but would prefer a VMInspector namespace over a class - unless you plan to make it instantiable, which doesn't seem to be the case.. > Source/JavaScriptCore/jit/JITExceptions.cpp:82 > +#if ENABLE(LLINT_C_LOOP) > + catchRoutine = reinterpret_cast<Instruction*>(catchPCForInterpreter->u.opcode); > +#else > + catchRoutine = handler->nativeCode.executableAddress(); > +#endif > + // mlam BEGIN > + #if mlam_PRINTF > + printf("\n mlam :: genericThrow() SET handler %p | catchPCForInterpreter = %p catchRoutine %p\n", handler, catchPCForInterpreter, catchRoutine); > + #endif // mlam_PRINTF > + // mlam END > + } else { > +#if ENABLE(LLINT_C_LOOP) > + // mlam orig catchRoutine = LLINT_ADDRESS_OF_GLUE(llint_ctiOpThrowNotCaught); > + // mlam COMPUTED_GOTO catchRoutine = const_cast<void*>(LLInt::Data::getOpcode(llint_ctiOpThrowNotCaught)); > + catchRoutine = LLInt::Data::getOpcodePtr(llint_ctiOpThrowNotCaught); > +#else > catchRoutine = FunctionPtr(ctiOpThrowNotCaught).value(); > +#endif > + // mlam BEGIN > + #if mlam_PRINTF > + printf("\n mlam :: genericThrow() SET NO handler | catchRoutine = %p\n", catchRoutine); > + #endif // mlam_PRINTF > + // mlam END > + } Why can't the catchRoutine be an "executable address" in the LLInt C loop as well? By "executable address" I of course mean a number that will cause the switch statement to take you to the relevant piece of code. > Source/JavaScriptCore/llint/LLIntData.cpp:64 > + if (!s_isInited) { > + s_exceptionInstructions = new Instruction[maxOpcodeLength + 1]; > + s_opcodeMap = new LLIntOpcode[numLLIntOpcodeIDs]; > +#if !ENABLE(LLINT_C_LOOP) > for (int i = 0; i < maxOpcodeLength + 1; ++i) > - m_exceptionInstructions[i].u.pointer = bitwise_cast<void*>(&llint_throw_from_slow_path_trampoline); > -#define OPCODE_ENTRY(opcode, length) m_opcodeMap[opcode] = bitwise_cast<void*>(&llint_##opcode); > + s_exceptionInstructions[i].u.pointer = bitwise_cast<void*>(&llint_throw_from_slow_path_trampoline); > +#define OPCODE_ENTRY(opcode, length) s_opcodeMap[opcode] = bitwise_cast<void*>(&llint_##opcode); > FOR_EACH_OPCODE_ID(OPCODE_ENTRY); > #undef OPCODE_ENTRY > + s_isInited = true; > +#endif // !ENABLE(LLINT_C_LOOP) > + } You're risking a race condition here. Either use pthread_once (or similar) or move this initialization into JSC::initializeThreading(). > Source/JavaScriptCore/llint/LLIntData.h:107 > + static bool s_isInited; isInited should be isInitialized. We generally prefer not to abbreviate. > Source/JavaScriptCore/llint/LLIntData.h:111 > + */ > + static bool s_isInited; > + static Instruction* s_exceptionInstructions; > + static LLIntOpcode* s_opcodeMap; > + // mlam END */ > }; Why is this change necessary? What's wrong with this being per-LLIntData-instance? Having it be per-LLIntData-instance means that you completely avoid initialization race conditions. > Source/JavaScriptCore/llint/LLIntSlowPaths.h:47 > - void* a; > - void* b; > + Instruction* a; > + ExecState* b; I believe we often return something other than Instruction* as the first word. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92 > +macro jumpToPC() > + if C_LOOP > + cxxNextOpcode > + else > + jmp [PC] // goto NEXT_INSTRUCTION; > + end > +end Why do you need this? Why isn't the translation of jmp <address> just: // the function you generate from the offlineasm enum LLIntGlobalLabels { _llint_op_something, // ... lots more }; void cLoop() { LLIntGlobalLabels nextPC; for (;;) { switch (nextPC) { case _llint_op_something: // translation of jmp [PC] nextPC = *bitwise_cast<LLIntGlobalLabels*>(PC); continue; // more things } } } > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:126 > + cxxCallSlowPath function, arg1, arg2 // call C function(arg1, arg2); For the record, I concur that having a special opcode for calls to C functions is right. I just don't agree that it's right for jmp, or for call when it's used for JS->JS calls.
Comment on attachment 157459 [details] Work in progress 4: added 64-bit support and some clean up. Passes JSC tests, v8 bench, and sunspider. On to layout tests next. View in context: https://bugs.webkit.org/attachment.cgi?id=157459&action=review >> Source/JavaScriptCore/interpreter/VMInspector.h:11 >> +class VMInspector { > > I like this, but would prefer a VMInspector namespace over a class - unless you plan to make it instantiable, which doesn't seem to be the case.. The usage cases for the VMInspector class is not baked yet. But I think I concur that there isn't a use case for instantiating it. I tend to prefer using classes as the smallest collection of functionality, and namespaces for arbitration between commonly named classes. In this case, I think of using "the VMInspector" as it is an object although there may ever be one instance / pseudo instance. But I don't mind changing it to be a namespace instead if I don't find any good reasons to keep it as a class when it's time to bake it. >> Source/JavaScriptCore/jit/JITExceptions.cpp:82 >> + } > > Why can't the catchRoutine be an "executable address" in the LLInt C loop as well? By "executable address" I of course mean a number that will cause the switch statement to take you to the relevant piece of code. It is an "executable address" in the lint loop. What I'm trying to do here is represent it in a way that the C compiler won't complain about. That said, I haven't tried really hard to make it real clean yet. Have just been aiming at getting it to work so far. Will revisit for applying a cleaner abstraction when things are fully functional. >> Source/JavaScriptCore/llint/LLIntData.cpp:64 >> + } > > You're risking a race condition here. Either use pthread_once (or similar) or move this initialization into JSC::initializeThreading(). OK, thanks for the tip. Will look into it. >> Source/JavaScriptCore/llint/LLIntData.h:111 >> }; > > Why is this change necessary? What's wrong with this being per-LLIntData-instance? Having it be per-LLIntData-instance means that you completely avoid initialization race conditions. Having the opcodeMap as a global singleton allows me to get its info from anywhere without needing a globalData pointer, and I encountered a few of such cases as I was trying to do this work. That is my main motivation. >> Source/JavaScriptCore/llint/LLIntSlowPaths.h:47 >> + ExecState* b; > > I believe we often return something other than Instruction* as the first word. I was letting the C compiler complain if there was a type conflict. So far, I didn't encounter any, but I will recheck to be sure before submitting my final patch. >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92 >> +end > > Why do you need this? > > Why isn't the translation of jmp <address> just: > > // the function you generate from the offlineasm > enum LLIntGlobalLabels { > _llint_op_something, > // ... lots more > }; > void cLoop() { > LLIntGlobalLabels nextPC; > for (;;) { > switch (nextPC) { > case _llint_op_something: > // translation of jmp [PC] > nextPC = *bitwise_cast<LLIntGlobalLabels*>(PC); > continue; > // more things > } > } > } Because there are local labels where a jump to it really means a jump as opposed to goto next opcode. I need to be able to distinguish between the 2. One alternative is to make the C loop backend aware that the branch target specifier is the vPC, and use that as the distinguishing factor. I can see if I can make that work later.
> >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92 > >> +end > > > > Why do you need this? > > > > Why isn't the translation of jmp <address> just: > > > > // the function you generate from the offlineasm > > enum LLIntGlobalLabels { > > _llint_op_something, > > // ... lots more > > }; > > void cLoop() { > > LLIntGlobalLabels nextPC; > > for (;;) { > > switch (nextPC) { > > case _llint_op_something: > > // translation of jmp [PC] > > nextPC = *bitwise_cast<LLIntGlobalLabels*>(PC); > > continue; > > // more things > > } > > } > > } > > Because there are local labels where a jump to it really means a jump as opposed to goto next opcode. I need to be able to distinguish between the 2. One alternative is to make the C loop backend aware that the branch target specifier is the vPC, and use that as the distinguishing factor. I can see if I can make that work later. But the offlineasm already distinguishes by those two uses of jmp. It already has the notion of local labels, and global labels.
(In reply to comment #8) > > >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92 > > >> +end > > > > > > Why do you need this? > > > > > > Why isn't the translation of jmp <address> just: > > > > > > // the function you generate from the offlineasm > > > enum LLIntGlobalLabels { > > > _llint_op_something, > > > // ... lots more > > > }; > > > void cLoop() { > > > LLIntGlobalLabels nextPC; > > > for (;;) { > > > switch (nextPC) { > > > case _llint_op_something: > > > // translation of jmp [PC] > > > nextPC = *bitwise_cast<LLIntGlobalLabels*>(PC); > > > continue; > > > // more things > > > } > > > } > > > } > > > > Because there are local labels where a jump to it really means a jump as opposed to goto next opcode. I need to be able to distinguish between the 2. One alternative is to make the C loop backend aware that the branch target specifier is the vPC, and use that as the distinguishing factor. I can see if I can make that work later. > > But the offlineasm already distinguishes by those two uses of jmp. It already has the notion of local labels, and global labels. The offlineasm distinguishes the declaration of the local and global labels. However, the jump sites uses the same vanilla imp instruction. In my case, I need the imp instruction to behave differently depending on which type of jump target we're jumping to.
(In reply to comment #9) > (In reply to comment #8) > > > >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:92 > > > >> +end > > > > > > > > Why do you need this? > > > > > > > > Why isn't the translation of jmp <address> just: > > > > > > > > // the function you generate from the offlineasm > > > > enum LLIntGlobalLabels { > > > > _llint_op_something, > > > > // ... lots more > > > > }; > > > > void cLoop() { > > > > LLIntGlobalLabels nextPC; > > > > for (;;) { > > > > switch (nextPC) { > > > > case _llint_op_something: > > > > // translation of jmp [PC] > > > > nextPC = *bitwise_cast<LLIntGlobalLabels*>(PC); > > > > continue; > > > > // more things > > > > } > > > > } > > > > } > > > > > > Because there are local labels where a jump to it really means a jump as opposed to goto next opcode. I need to be able to distinguish between the 2. One alternative is to make the C loop backend aware that the branch target specifier is the vPC, and use that as the distinguishing factor. I can see if I can make that work later. > > > > But the offlineasm already distinguishes by those two uses of jmp. It already has the notion of local labels, and global labels. > > The offlineasm distinguishes the declaration of the local and global labels. However, the jump sites uses the same vanilla imp instruction. In my case, I need the imp instruction to behave differently depending on which type of jump target we're jumping to. It also distinguishes between references to labels. You *always know* whether you're jumping to a local or global label.
Created attachment 158355 [details] work in progress 5: more bug fixes, some clean up, and incorporated some of Filip's feedback.
Created attachment 158356 [details] work in progress 6: added a missing new file. Now down to 2 crashes and 1 test failure left to investigate.
Created attachment 158736 [details] Example of generated 32-bit llint C loop code.
Created attachment 158737 [details] Example of generated 64-bit llint C loop code.
Created attachment 158852 [details] work in progress 7: more bug fixes.
> OFFLINE_ASM_OPCODE_LABEL(op_create_this) > r0.i = *CAST<intptr_t*>(fp.i8p - 32); // this<t0> = cfr.Callee; > r2.i = *CAST<intptr_t*>(r0.i8p + 40); // inheritor<t2> = this<t0>.cachedInheritorID; I see a couple issues here. (1) r0 and r2 are declared to be interpreter-global. This will defeat local register allocation, possibly causing performance problems. Can you test the performance of this interpreter, as compared to the existing .cpp interpreter, and post the results? Performance is not a top priority here, but we'd like to pay it some mind. (2) This code is a bit cryptic. Assembly forces you to be cryptic. But we're auto-generating C++ here, so we can be much more explanatory. For example, when the existing .cpp interpreter wants to read the callee register, it just says "JSFunction* constructor = jsCast<JSFunction*>(callFrame->callee())". Why is that kind of code not possible here? Are we going out of our way to mimic the assembly generated by our assembly back-ends for some reason? (3) Comments. I tend to agree that this code is cryptic, and could use some explanation. However, the comments are equally cryptic. I don't know what <> annotation means, and I don't know what these numbered t's are. The original code reads: loadp Callee[cfr], t0 Why can't our C++ code have variables named "cfr", "callee" and "t0", and perform an operation like "Register t0 = cfr[callee]"?
(In reply to comment #16) > > OFFLINE_ASM_OPCODE_LABEL(op_create_this) > > r0.i = *CAST<intptr_t*>(fp.i8p - 32); // this<t0> = cfr.Callee; > > r2.i = *CAST<intptr_t*>(r0.i8p + 40); // inheritor<t2> = this<t0>.cachedInheritorID; > > I see a couple issues here. > > (1) r0 and r2 are declared to be interpreter-global. This will defeat local register allocation, possibly causing performance problems. Can you test the performance of this interpreter, as compared to the existing .cpp interpreter, and post the results? > > Performance is not a top priority here, but we'd like to pay it some mind. > > (2) This code is a bit cryptic. Assembly forces you to be cryptic. But we're auto-generating C++ here, so we can be much more explanatory. For example, when the existing .cpp interpreter wants to read the callee register, it just says "JSFunction* constructor = jsCast<JSFunction*>(callFrame->callee())". Why is that kind of code not possible here? Are we going out of our way to mimic the assembly generated by our assembly back-ends for some reason? If you're generating C++ code from assembly code, I'm not sure how you're going to avoid it being cryptic. In particular, the offlineasm blows away offsetof operations early, before it gets to the backend. We could change that, but that would add an entirely separate path to the offlineasm and hence would defeat the goal of reducing the amount of code we want to maintain. As well, I don't think we'll ever be touching the C++ code generated by the offlineasm, just as we currently almost never touch the assembly code generated by the offlineasm. > > (3) Comments. I tend to agree that this code is cryptic, and could use some explanation. However, the comments are equally cryptic. I don't know what <> annotation means, and I don't know what these numbered t's are. > > The original code reads: > > loadp Callee[cfr], t0 > > Why can't our C++ code have variables named "cfr", "callee" and "t0", and perform an operation like "Register t0 = cfr[callee]"?
Here's a specific suggestion for making this more readable. Instead of: r4.i = *CAST<intptr_t*>(r0.i8p); // next<a5> = result<a3>.ptr; *CAST<intptr_t*>(r1.i8p + 1680) = r4.i; // globalData<a4>.MySizeClass.FirstFreeCell = next<a5>; How about: loadp(r4, address(r0)); storei(address(r1, 1680), r4);
(In reply to comment #16) > > OFFLINE_ASM_OPCODE_LABEL(op_create_this) > > r0.i = *CAST<intptr_t*>(fp.i8p - 32); // this<t0> = cfr.Callee; > > r2.i = *CAST<intptr_t*>(r0.i8p + 40); // inheritor<t2> = this<t0>.cachedInheritorID; > > I see a couple issues here. > > (1) r0 and r2 are declared to be interpreter-global. This will defeat local register allocation, possibly causing performance problems. Can you test the performance of this interpreter, as compared to the existing .cpp interpreter, and post the results? > > Performance is not a top priority here, but we'd like to pay it some mind. The pseudo registers do need to be interpreter global because by definition (like CPU regs), they need to carry their state across to subsequent asm "instruction"s. Will check the performance. > (2) This code is a bit cryptic. Assembly forces you to be cryptic. But we're auto-generating C++ here, so we can be much more explanatory. For example, when the existing .cpp interpreter wants to read the callee register, it just says "JSFunction* constructor = jsCast<JSFunction*>(callFrame->callee())". Why is that kind of code not possible here? Are we going out of our way to mimic the assembly generated by our assembly back-ends for some reason? It is cryptic because by the time the backend gets the info, all I know about are registers, offsets, and what operation to perform on them. For example, I don't have data to tell me that I'm fetching the callee, only that I am to deref the cfr at a certain numeric constant offset and load that value. > (3) Comments. I tend to agree that this code is cryptic, and could use some explanation. However, the comments are equally cryptic. I don't know what <> annotation means, and I don't know what these numbered t's are. > > The original code reads: > > loadp Callee[cfr], t0 > > Why can't our C++ code have variables named "cfr", "callee" and "t0", and perform an operation like "Register t0 = cfr[callee]"? The backend sees: loadp [cfr, -32], t0 I could do extra work to recognize that -32 is the offset for callee on the 32 bit version when applied to the cfr. How much effort do you want to invest in generating more readable code considering that we will rarely have to debug at this level. As for the comments, I use "this<t0>" to denote a value named "this" stored in register t0. It's t0 because the comment is a const string written in the context of the asm code. The backend later maps it to r0. I could keep it as t0 to make it map better.
(In reply to comment #18) > Here's a specific suggestion for making this more readable. Instead of: > > r4.i = *CAST<intptr_t*>(r0.i8p); // next<a5> = result<a3>.ptr; > *CAST<intptr_t*>(r1.i8p + 1680) = r4.i; // globalData<a4>.MySizeClass.FirstFreeCell = next<a5>; > > How about: > > loadp(r4, address(r0)); > storei(address(r1, 1680), r4); Are you suggesting that I make the output look more like ASM code? I can do that if its preferable.
(In reply to comment #20) > (In reply to comment #18) > > Here's a specific suggestion for making this more readable. Instead of: > > > > r4.i = *CAST<intptr_t*>(r0.i8p); // next<a5> = result<a3>.ptr; > > *CAST<intptr_t*>(r1.i8p + 1680) = r4.i; // globalData<a4>.MySizeClass.FirstFreeCell = next<a5>; > > > > How about: > > > > loadp(r4, address(r0)); > > storei(address(r1, 1680), r4); > > Are you suggesting that I make the output look more like ASM code? I can do that if its preferable. I vote for not making this change; I think it's good as-is. In my experience, we rarely look at the generated code. And when we do, I think it's better to see exactly what the code is doing, which your form is good for.
> How much effort do you want to invest in generating more readable code considering that we will rarely have to debug at this level. I hear you and Phil both saying that debug-ability doesn't matter here. But you invented a whole new syntax for comment-embedded pseudo-assembly because you said you couldn't debug without it. What gives? FWIW, when I read this code, the comment-embedded pseudo-asssembly makes things harder to read, because now I'm speaking in three languages (LLInt assembly, c++, and pseudo-assembly) instead of two, and one of these languages has no specification and no debugger. > Are you suggesting that I make the output look more like ASM code? I can do that if its preferable. My main suggestion is that any comment you would want to put on the right of the "//" would serve us better as code on the left of the "//". If you want to name an offset or a register on the right, just name it in the code on the left instead. Let's not use comments as a crutch to excuse unreadable code. On the other hand, if you think the code is perfectly readable without the comments, or readability doesn't matter for some special reason in this case, please remove the comments.
The change set for adding the llint C loop backend was broken up into several bugs. For reference, here are the other commits that were preceeded this final one: Adding support for adding LLInt opcode extensions. https://bugs.webkit.org/show_bug.cgi?id=95277 Refactoring LLInt::Data to be a singleton https://bugs.webkit.org/show_bug.cgi?id=95316 Render unto #ifdef's that which belongs to them. https://bugs.webkit.org/show_bug.cgi?id=95482 Refactor LLInt and supporting code in preparation for the C Loop backend. https://bugs.webkit.org/show_bug.cgi?id=95531
Created attachment 161822 [details] Fix.
Attachment 161822 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/llint/LLIntCLoop.h:41: llint_unused is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:66: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:69: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:71: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:76: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:79: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:134: Missing spaces around && [whitespace/operators] [3] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:139: Missing spaces around && [whitespace/operators] [3] Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:400: Missing spaces around && [whitespace/operators] [3] Total errors found: 9 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
The few style violations were intentional. They were: 1. llint_unused uses underscore. This was intentional to keep it consistent with how opcodes are named. 2. Use of more than 1 space between certain words in a comment block. This was because I was expressing a column aligned table of info which necessitates the use of space for alignment. 3. No space around &&. The style checker thought it's a logical AND operator, but it's actually a "get the address of a label" operator, and it does not make sense to have spaces around it where it is used.
(In reply to comment #26) > The few style violations were intentional. They were: > 1. llint_unused uses underscore. This was intentional to keep it consistent with how opcodes are named. > 2. Use of more than 1 space between certain words in a comment block. This was because I was expressing a column aligned table of info which necessitates the use of space for alignment. > 3. No space around &&. The style checker thought it's a logical AND operator, but it's actually a "get the address of a label" operator, and it does not make sense to have spaces around it where it is used. Yeah, I just looked at them. The address of label thing is a little annoying, that said: does this work in compiler that don't support computed goto?
(In reply to comment #26) > The few style violations were intentional. They were: > 1. llint_unused uses underscore. This was intentional to keep it consistent with how opcodes are named. > 2. Use of more than 1 space between certain words in a comment block. This was because I was expressing a column aligned table of info which necessitates the use of space for alignment. > 3. No space around &&. The style checker thought it's a logical AND operator, but it's actually a "get the address of a label" operator, and it does not make sense to have spaces around it where it is used. Yeah, for changes in parts of the JIT and LLInt we end up getting loads of these sorts of warnings and we pretty much always ignore them.
(In reply to comment #27) > (In reply to comment #26) > > The few style violations were intentional. They were: > > 1. llint_unused uses underscore. This was intentional to keep it consistent with how opcodes are named. > > 2. Use of more than 1 space between certain words in a comment block. This was because I was expressing a column aligned table of info which necessitates the use of space for alignment. > > 3. No space around &&. The style checker thought it's a logical AND operator, but it's actually a "get the address of a label" operator, and it does not make sense to have spaces around it where it is used. > > Yeah, I just looked at them. The address of label thing is a little annoying, that said: does this work in compiler that don't support computed goto? It should. That was the design, and the code has paths for both.
(In reply to comment #29) > (In reply to comment #27) > > (In reply to comment #26) > > > The few style violations were intentional. They were: > > > 1. llint_unused uses underscore. This was intentional to keep it consistent with how opcodes are named. > > > 2. Use of more than 1 space between certain words in a comment block. This was because I was expressing a column aligned table of info which necessitates the use of space for alignment. > > > 3. No space around &&. The style checker thought it's a logical AND operator, but it's actually a "get the address of a label" operator, and it does not make sense to have spaces around it where it is used. > > > > Yeah, I just looked at them. The address of label thing is a little annoying, that said: does this work in compiler that don't support computed goto? > > It should. That was the design, and the code has paths for both. Cool. How's performance compare vs. the old C interpreter? (It doesn't matter too much, but it would be nice to know)
Comment on attachment 161822 [details] Fix. Clearing flags on attachment: 161822 Committed r127374: <http://trac.webkit.org/changeset/127374>
All reviewed patches have been landed. Closing bug.
Created attachment 163231 [details] Comparison of layout test results for C++ llint, ASM llint, and Classic interpreter. Here's an excerpt (from the attached file) of just the failed results which are relevant to the C++ llint: Summary of layout test runs: Failures Comments =========================== ====================================== ======== Regressions: Unexpected text failures : (11) accessibility/accessibility-node-memory-management.html C++ llint Classic Consistently fails w/ 100 iterations. fast/events/remove-target-with-shadow-in-drag.html C++ llint ASM llint Consistently fails w/ 100 iterations. fast/spatial-navigation/snav-container-white-space.html C++ llint ASM llint Classic Consistently fails w/ 100 iterations. fast/spatial-navigation/snav-div-overflow-scrol-hidden.html C++ llint ASM llint Classic Consistently fails w/ 100 iterations. fast/spatial-navigation/snav-imagemap-overlapped-areas.html C++ llint ASM llint Classic Consistently fails w/ 100 iterations. fast/workers/worker-close-more.html C++ llint ASM llint Classic Consistently fails. fast/workers/worker-lifecycle.html C++ llint ASM llint Classic Passes when run by itself. jquery/manipulation.html C++ llint Flaky. Passes when run alone. platform/mac/editing/spelling/removing-underline-after-accepting-autocorrection-using-punctuation.html C++ llint ASM llint Consistently fails w/ 100 iterations. platform/mac/fast/loader/file-url-mimetypes-3.html C++ llint ASM llint Classic Consistently fails w/ 100 iterations. svg/text/non-bmp-positioning-lists.svg C++ llint ASM llint Classic Consistently fails w/ 100 iterations. Regressions: Unexpected crashes : (2) fast/profiler/profiling-from-a-nested-location.html C++ llint Flaky when run alone w/ 100 iterations. inspector/extensions/extensions-eval.html C++ llint Passes when run alone w/ 100 iterations. Regressions: Unexpected timeouts : (1) fast/js/random-array-gc-stress.html C++ llint ASM llint Classic Passes on JIT build or when timeout is removed. Comparing the C++ llint failures with the ASM llint and the Classic intepreter, it doesn't look like there are new regressions. In all cases, the failures either already exists in the ASM llint or the classic interpreter, or the failure is intermittent (could be test flakiness). Certainly, all of these should be investigated, but the only tests that could potentially be a C++ llint only issue are: jquery/manipulation.html fast/profiler/profiling-from-a-nested-location.html I will a bug to track this.
(In reply to comment #33) > I will a bug to track this. Bug filed: https://bugs.webkit.org/show_bug.cgi?id=96333