Bug 213566 – Bytecode UseDef should be aware of checkpoints

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Bytecode UseDef should be aware of checkpoints

[Keith Miller]

Created attachment 402663 [details]
Patch

[Keith Miller]

Created attachment 402664 [details]
Patch

[Keith Miller]

Created attachment 402669 [details]
Patch

[Keith Miller]

Created attachment 402689 [details]
Patch

[Saam Barati]

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.

[Keith Miller]

Created attachment 403644 [details]
Patch

[Saam Barati]

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?

[Keith Miller]

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.

[Keith Miller]

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`

[Keith Miller]

Created attachment 403710 [details]
Patch for landing

[Keith Miller]

rdar://64639220

[Aakash Jain]

(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

[Keith Miller]

(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.

[EWS]

Found 1 new test failure: inspector/dom/domutilities-csspath.html

[Keith Miller]

(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 😬

[Keith Miller]

Created attachment 403744 [details]
Patch for landing

[EWS]

Committed r264049: <https://trac.webkit.org/changeset/264049>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403744 [details].