Bytecode UseDef should be aware of checkpoints
Created attachment 402663 [details] Patch
Created attachment 402664 [details] Patch
Created attachment 402669 [details] Patch
Created attachment 402689 [details] Patch
Comment on attachment 402689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402689&action=review > Source/JavaScriptCore/bytecode/BytecodeIndex.h:37 > +using Checkpoint = uint8_t; nit: Maybe CheckpointIndex? > Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:64 > + for (Checkpoint checkpoint = instructions.at(bytecodeIndex)->numberOfCheckpoints(); checkpoint--;) { just for sanity reading this too, can you assert numberOfCheckpoints is < length of the bytecode > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:293 > + case op_iterator_open: { > + auto bytecode = instruction->as<OpIteratorOpen>(); > + functor(OpIteratorOpen::symbolCall, bytecode.m_symbolIterator); > + functor(OpIteratorOpen::symbolCall, bytecode.m_iterable); > + functor(OpIteratorOpen::getNext, bytecode.m_iterator); > + return; > + } this feels like a curious implementation. Why not just branch on the checkpoint being passed in here to see if it's the right checkpoint? Instead of branching on the caller side to see if you're dealing with the right checkpoint. I think it's a much more obviously correct implementation.
Created attachment 403644 [details] Patch
Comment on attachment 403644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403644&action=review r=me > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:49 > + auto functor = [&] (Checkpoint target, VirtualRegister operand) { Calling this functor and the other functorImpl are weird. I’d keep the parameter functor, and maybe call this “useAtCheckpoint” or something? > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:57 > + functor(noCheckpoints, VirtualRegister { base - i }); Why not functorImpl? > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:64 > + functor(noCheckpoints, VirtualRegister { lastArg + i }); Why not functorImpl? > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:66 > + functor(noCheckpoints, scopeRegister); Why not functorImpl? > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:266 > + useAtEachCheckpoint(bytecode.m_callee, bytecode.m_thisValue, bytecode.m_arguments); Is this just so you can add the static assert to the macro? If so, I’m not sure it’s accomplishing much. I kinda like calling the macro instead and removing the static assert > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:292 > + auto bytecode = instruction->as<OpIteratorOpen>(); Alternatively to my suggestion on naming of functor and functorImpl to useAtCheckpoint, you could totally just switch on the bytecode index here. Might be a nicer style tbh and is in line with switching on the bytecode itself. > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:318 > + functor(noCheckpoints, VirtualRegister { base - i }); Why not functorImpl? > Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:343 > + auto functor = [&] (Checkpoint target, VirtualRegister operand) { Ditto about naming > Source/JavaScriptCore/dfg/DFGForAllKills.h:90 > + } else if (before.bytecodeIndex().checkpoint()) { Do we have an assert anywhere that no temp can be live-in to checkpoint 0?
Comment on attachment 403644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403644&action=review >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:49 >> + auto functor = [&] (Checkpoint target, VirtualRegister operand) { > > Calling this functor and the other functorImpl are weird. I’d keep the parameter functor, and maybe call this “useAtCheckpoint” or something? Sure, seems reasonable. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:57 >> + functor(noCheckpoints, VirtualRegister { base - i }); > > Why not functorImpl? Done. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:64 >> + functor(noCheckpoints, VirtualRegister { lastArg + i }); > > Why not functorImpl? Done. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:66 >> + functor(noCheckpoints, scopeRegister); > > Why not functorImpl? Done. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:266 >> + useAtEachCheckpoint(bytecode.m_callee, bytecode.m_thisValue, bytecode.m_arguments); > > Is this just so you can add the static assert to the macro? If so, I’m not sure it’s accomplishing much. I kinda like calling the macro instead and removing the static assert The macro doesn't work for all the checkpoint bytecodes since it includes the switch label in it. I figured it was better to be consistent among the checkpoint bytecodes. I don't have strong feelings though. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:292 >> + auto bytecode = instruction->as<OpIteratorOpen>(); > > Alternatively to my suggestion on naming of functor and functorImpl to useAtCheckpoint, you could totally just switch on the bytecode index here. Might be a nicer style tbh and is in line with switching on the bytecode itself. Yeah, my only thought was that seemed more verbose. If you think it's better though, I'll change it. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:318 >> + functor(noCheckpoints, VirtualRegister { base - i }); > > Why not functorImpl? Done. >> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:343 >> + auto functor = [&] (Checkpoint target, VirtualRegister operand) { > > Ditto about naming Done. >> Source/JavaScriptCore/dfg/DFGForAllKills.h:90 >> + } else if (before.bytecodeIndex().checkpoint()) { > > Do we have an assert anywhere that no temp can be live-in to checkpoint 0? No, I think we only assert nothing is live out and isn't live at the prologue.
Comment on attachment 403644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403644&action=review >>> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:49 >>> + auto functor = [&] (Checkpoint target, VirtualRegister operand) { >> >> Calling this functor and the other functorImpl are weird. I’d keep the parameter functor, and maybe call this “useAtCheckpoint” or something? > > Sure, seems reasonable. I killed this lambda so there's just the argument now, which still has the name `functor`. >>> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:292 >>> + auto bytecode = instruction->as<OpIteratorOpen>(); >> >> Alternatively to my suggestion on naming of functor and functorImpl to useAtCheckpoint, you could totally just switch on the bytecode index here. Might be a nicer style tbh and is in line with switching on the bytecode itself. > > Yeah, my only thought was that seemed more verbose. If you think it's better though, I'll change it. I think I'm going to add a useAtEachCheckpointStartingWith rather than have switches that have a bunch of `FALLTHROUGH` everywhere. >>> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:343 >>> + auto functor = [&] (Checkpoint target, VirtualRegister operand) { >> >> Ditto about naming > > Done. I renamed this lambda to `defAt` and kept the argument as `functor`
Created attachment 403710 [details] Patch for landing
rdar://64639220
(In reply to Keith Miller from comment #10) > Created attachment 403710 [details] > Patch for landing Can you please check if the test failures in following build are caused by this patch: https://ews-build.webkit.org/#/builders/28/builds/3309 https://ews-build.webkit.org/#/builders/30/builds/13355 https://ews-build.webkit.org/#/builders/31/builds/13509
(In reply to Aakash Jain from comment #12) > (In reply to Keith Miller from comment #10) > > Created attachment 403710 [details] > > Patch for landing > Can you please check if the test failures in following build are caused by > this patch: > > https://ews-build.webkit.org/#/builders/28/builds/3309 > https://ews-build.webkit.org/#/builders/30/builds/13355 > https://ews-build.webkit.org/#/builders/31/builds/13509 Seems unlikely given the patch before was green and I only did minor refactors. I'll double check though.
Found 1 new test failure: inspector/dom/domutilities-csspath.html
(In reply to Keith Miller from comment #13) > (In reply to Aakash Jain from comment #12) > > (In reply to Keith Miller from comment #10) > > > Created attachment 403710 [details] > > > Patch for landing > > Can you please check if the test failures in following build are caused by > > this patch: > > > > https://ews-build.webkit.org/#/builders/28/builds/3309 > > https://ews-build.webkit.org/#/builders/30/builds/13355 > > https://ews-build.webkit.org/#/builders/31/builds/13509 > > Seems unlikely given the patch before was green and I only did minor > refactors. I'll double check though. Whoops, looks like there was a >= that should have been a <=... I guess forgot to test before uploading 😬
Created attachment 403744 [details] Patch for landing
Committed r264049: <https://trac.webkit.org/changeset/264049> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403744 [details].