[webkit-reviews] review granted: [Bug 213566] Bytecode UseDef should be aware of checkpoints : [Attachment 403644] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 213566: Bytecode UseDef should be aware of checkpoints
https://bugs.webkit.org/show_bug.cgi?id=213566

Attachment 403644: Patch

https://bugs.webkit.org/attachment.cgi?id=403644&action=review




--- Comment #7 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 403644
  --> https://bugs.webkit.org/attachment.cgi?id=403644
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?