Since it well generalize patchpoint over all the JIT tiers, I think it is useful for non DOMJIT use cases. It is somewhat well defined compiler injection to JSC.
Created attachment 310647 [details] Patch
Comment on attachment 310647 [details] Patch Attachment 310647 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3776288 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-buffered.html
Created attachment 310651 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 310647 [details] Patch Attachment 310647 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3777010 New failing tests: media/W3C/video/currentSrc/currentSrc_nonempty_after_setting_src.html
Created attachment 310662 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 310769 [details] Patch
Comment on attachment 310769 [details] Patch I think just choosing the name Patchpoint might be confusing w.r.t B3’s patchpoint. I don’t have a suggestion off the top of my head but I’ll think about it.
(In reply to Saam Barati from comment #7) > Comment on attachment 310769 [details] > Patch > > I think just choosing the name Patchpoint might be confusing w.r.t B3’s > patchpoint. I don’t have a suggestion off the top of my head but I’ll think > about it. How about "InjectableCodeGenerator" / "InjectableCompiler" / "PatchpointCompiler" / "PatchpointCodeGenerator"?
(In reply to Yusuke Suzuki from comment #8) > (In reply to Saam Barati from comment #7) > > Comment on attachment 310769 [details] > > Patch > > > > I think just choosing the name Patchpoint might be confusing w.r.t B3’s > > patchpoint. I don’t have a suggestion off the top of my head but I’ll think > > about it. > > How about "InjectableCodeGenerator" / "InjectableCompiler" / > "PatchpointCompiler" / "PatchpointCodeGenerator"? Ping?
Comment on attachment 310769 [details] Patch I like the idea of putting this functionality at the top-level and dissociating it from "DOM". I'm just not sure I like spreading the use of the name "patchpoint". It has a pretty specific meaning in B3, which is subtly different from this. The term for this that I've seen used elsewhere is "snippet". The patchpoint is providing a snippet, and the "patchpoint generator" is really a "snippet compiler". The benefit of renaming DOMPatchpoint to Snippet and DOMPatchpointGenerator to SnippetCompiler is threefold: - It completely avoids any name collisions with B3. - It avoids people expecting B3's patchpoints and these things to behave exactly the same. They are similar but not identical and they are allowed to diverge from each other. - I think that "snippet" is the better term. If I had a chance to do B3 over again, I would have used "snippet". RS=me to move this to the top level. I'm not completely opposed to continuing to use the patchpoint name for this functionality, but I think that calling it snippet would be nicer.
(In reply to Yusuke Suzuki from comment #9) > (In reply to Yusuke Suzuki from comment #8) > > (In reply to Saam Barati from comment #7) > > > Comment on attachment 310769 [details] > > > Patch > > > > > > I think just choosing the name Patchpoint might be confusing w.r.t B3’s > > > patchpoint. I don’t have a suggestion off the top of my head but I’ll think > > > about it. > > > > How about "InjectableCodeGenerator" / "InjectableCompiler" / > > "PatchpointCompiler" / "PatchpointCodeGenerator"? > > Ping? Ha! I didn't see this thread before clicking "Review Patch". What do you think of Snippet/SnippetCompiler? It's so concise!
+1 for Snippet/SnippetCompiler
Sounds super nice! I'll rename to Snippet and SnippetCompiler :D
Created attachment 311421 [details] Patch Patch for landing
Comment on attachment 311421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311421&action=review lgtm. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10270 > + FTLSnippetParams domJITParams(*state, params, node, nullptr, WTFMove(regs), WTFMove(gpScratch), WTFMove(fpScratch)); I wonder if FTLLowerDFGToB3 could just include FTLSnippetParams, which means you could just say SnippetParams here, and you'd get the FTL one. We do this game with FTL::JITCode (which inherits from JSC::JITCode). I think it's mostly not confusing, but that's just me. I like being terse. But maybe it's confusing to others. I don't feel strongly about this. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10367 > + Vector<SnippetParams::Value> regs; If I'm right, you'd still say SnippetParams here - you'd be referring to FTL::SnippetParams, but that publicly extends JSC::SnippetParams, so this just works. > Source/JavaScriptCore/ftl/FTLSnippetParams.h:38 > +class FTLSnippetParams : public SnippetParams { What about "class SnippetParams : public JSC::SnippetParams"? This would mirror how JITCode works, for example. I would only want us to do this if it doesn't cause us to have to say JSC::SnippetParams everywhere. OTOH I think it's OK to say FTL::SnippetParams instead of FTLSnippetParams.
Comment on attachment 311421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311421&action=review Thanks! >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10270 >> + FTLSnippetParams domJITParams(*state, params, node, nullptr, WTFMove(regs), WTFMove(gpScratch), WTFMove(fpScratch)); > > I wonder if FTLLowerDFGToB3 could just include FTLSnippetParams, which means you could just say SnippetParams here, and you'd get the FTL one. We do this game with FTL::JITCode (which inherits from JSC::JITCode). I think it's mostly not confusing, but that's just me. I like being terse. But maybe it's confusing to others. I don't feel strongly about this. Agreed. That's good call. This leads to unconfusing code, like, what is the difference between SnippetParams and FTLSnippetParams :) Fixed. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10367 >> + Vector<SnippetParams::Value> regs; > > If I'm right, you'd still say SnippetParams here - you'd be referring to FTL::SnippetParams, but that publicly extends JSC::SnippetParams, so this just works. Yup right. I don't think this SnippetParams will be extended in different tiers because Snippet's code generator need to receive SnippetParams::Value vector for all the tiers, but renaming FTLSnippetParams to FTL::SnippetParams leads to more readable code. We don't need to confuse the difference between FTLSnippetParams and JSC::SnippetParams :). >> Source/JavaScriptCore/ftl/FTLSnippetParams.h:38 >> +class FTLSnippetParams : public SnippetParams { > > What about "class SnippetParams : public JSC::SnippetParams"? > > This would mirror how JITCode works, for example. I would only want us to do this if it doesn't cause us to have to say JSC::SnippetParams everywhere. OTOH I think it's OK to say FTL::SnippetParams instead of FTLSnippetParams. Good call. While we need to keep the name AccessCaseSnippetParams (Since it is used in all the tiers' IC and IC does not have specific namespace.), we can rename FTLSnippetParams to FTL::SnippetParams. And we can also rename DFGSnippetParams to DFG::SnippetParams. Fixed.
Committed r217523: <http://trac.webkit.org/changeset/217523>
<rdar://problem/32479824>