Patch coming.
<rdar://problem/39022125>
Created attachment 336825 [details] proposed patch.
Comment on attachment 336825 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=336825&action=review r=me with some comments. > Source/JavaScriptCore/b3/B3LowerMacros.cpp:531 > + jumpTable[tableIndex] = linkBuffer.locationOf(*labels[labelIndex++], tag); For the two changes above: do we ever generate other jumps to these labels which need a tag? Say we have: switch(i) { case 1: oops: // ... case 2: switch (j) { case 4: // ... case 5: goto oops; } case 3: // ... } So you've tagged the inner and outer switch differently, and the dispatch at the top of each switch has the same tag... What does case 1 and label oops end up doing? I'm guessing labels are always direct jumps so we're OK? Let's talk it through tomorrow, I just want to make sure I understand! > Source/JavaScriptCore/b3/B3LowerMacrosAfterOptimizations.cpp:126 > + functionAddress = m_insertionSet.insert<ConstPtrValue>(m_index, m_origin, tagCFunctionPtr(floorf, B3CCallPtrTag)); For the 4 above: I think your other idiom with tagCFunctionPtr<...>(f, tag) is easier to grok that this one. > Source/JavaScriptCore/b3/testb3.cpp:10236 > + root->appendNew<ConstPtrValue>(proc, Origin(), tagCFunctionPtr<void*>(simpleFunction, B3CCallPtrTag)), No signature here and below? > Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:138 > + jit.move(CCallHelpers::TrustedImmPtr(B3CCallPtrTag), ptrTagRegister); I thought you used TrustedImm64 for the tags?
Comment on attachment 336825 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=336825&action=review Thanks for the review. Clarifications and responses below. >> Source/JavaScriptCore/b3/B3LowerMacros.cpp:531 >> + jumpTable[tableIndex] = linkBuffer.locationOf(*labels[labelIndex++], tag); > > For the two changes above: do we ever generate other jumps to these labels which need a tag? > > Say we have: > > switch(i) { > case 1: > oops: > // ... > case 2: > switch (j) { > case 4: > // ... > case 5: > goto oops; > } > case 3: > // ... > } > > So you've tagged the inner and outer switch differently, and the dispatch at the top of each switch has the same tag... What does case 1 and label oops end up doing? I'm guessing labels are always direct jumps so we're OK? Let's talk it through tomorrow, I just want to make sure I understand! We're only tagging the switch cases (and default) here. If there's a nested switch, it will have its own dispatch, and therefore, its own tag, and its cases will be linked separately. We don't have to worry about it here. Secondly, there's no "goto" in the ECMAScript spec (at least I don't see one): https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf. >> Source/JavaScriptCore/b3/B3LowerMacrosAfterOptimizations.cpp:126 >> + functionAddress = m_insertionSet.insert<ConstPtrValue>(m_index, m_origin, tagCFunctionPtr(floorf, B3CCallPtrTag)); > > For the 4 above: I think your other idiom with tagCFunctionPtr<...>(f, tag) is easier to grok that this one. There is no other idiom. By default, tagCFunctionPtr(ptr, tag) returns a pointer of the same type as ptr i.e. the function pointer type of the function passed to it. However, as a convenience, we also have a form of tagCFunctionPtr that bitwise_casts the result to a different pointer type if desired e.g. tagCFunctionPtr<void*>(ptr, tag) returns a void*. In the above 4, there's no reason to return a different pointer type than the function pointer we're tagging. > Source/JavaScriptCore/b3/B3MathExtras.cpp:60 > + auto* powDouble = tagCFunctionPtr<double (*)(double, double)>(pow, B3CCallPtrTag); FYI, I'm using the tagCFunctionPtr<double (*)(double, double)>(...) form here so that I can use auto* to declare powDouble. >> Source/JavaScriptCore/b3/testb3.cpp:10236 >> + root->appendNew<ConstPtrValue>(proc, Origin(), tagCFunctionPtr<void*>(simpleFunction, B3CCallPtrTag)), > > No signature here and below? No signature needed. The original code wanted a void*, we choose the form that applies the bitwise_cast to void*. >> Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:138 >> + jit.move(CCallHelpers::TrustedImmPtr(B3CCallPtrTag), ptrTagRegister); > > I thought you used TrustedImm64 for the tags? PtrTag is a uintptr_t i.e. pointer size. Hence, TrustedImmPtr.
(In reply to Mark Lam from comment #4) > Comment on attachment 336825 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336825&action=review > > Thanks for the review. Clarifications and responses below. > > >> Source/JavaScriptCore/b3/B3LowerMacros.cpp:531 > >> + jumpTable[tableIndex] = linkBuffer.locationOf(*labels[labelIndex++], tag); > > > > For the two changes above: do we ever generate other jumps to these labels which need a tag? > > > > Say we have: > > > > switch(i) { > > case 1: > > oops: > > // ... > > case 2: > > switch (j) { > > case 4: > > // ... > > case 5: > > goto oops; > > } > > case 3: > > // ... > > } > > > > So you've tagged the inner and outer switch differently, and the dispatch at the top of each switch has the same tag... What does case 1 and label oops end up doing? I'm guessing labels are always direct jumps so we're OK? Let's talk it through tomorrow, I just want to make sure I understand! > > We're only tagging the switch cases (and default) here. If there's a nested > switch, it will have its own dispatch, and therefore, its own tag, and its > cases will be linked separately. We don't have to worry about it here. OK makes sense. > Secondly, there's no "goto" in the ECMAScript spec (at least I don't see > one): > https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf. Totally is! https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/label I'm more asking about our IR constructs, not JS. The IR could allow goto without JS having it, at least LLVM IR allows a bunch of things C++ (even with extensions) doesn't.
(In reply to JF Bastien from comment #5) > > Secondly, there's no "goto" in the ECMAScript spec (at least I don't see > > one): > > https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf. > > Totally is! > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/ > label > > I'm more asking about our IR constructs, not JS. The IR could allow goto > without JS having it, at least LLVM IR allows a bunch of things C++ (even > with extensions) doesn't. Good point. That would be handled by a Label and a jump in our JIT. It's completely independent of this switch dispatch. Hence, no need to be concerned about it.
Landed in r230098: <http://trac.webkit.org/r230098>.