Bug 213106 – Bytecode liveness should be aware of checkpoints

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

...

[Tadeu Zagallo]

<rdar://problem/63416838>

[Tadeu Zagallo]

Created attachment 401697 [details]
Patch

[Tadeu Zagallo]

Created attachment 401698 [details]
Patch

[Saam Barati]

Comment on attachment 401698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401698&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        Bytecode liveness should be able to express that checkpoints can use use a value that was written to at
> +        an earlier point in that bytecode's execution. E.g. in op_iterator_open, by the time we get to the getNext
> +        checkpoint, we rely that m_iterator already contains the iterator we just allocated. Prior to this patch,
> +        that value would not be properly set when we OSR exit to OpIteratorOpen::getNext.

what do you mean by "not properly set"? Why? Don't we already take into account checkpoint liveness in DFG? Why is this needed here if we already do that?

[Saam Barati]

Is the plan here to do what I suggested on Slack yesterday to simplify the approach? Or keep it as is?

[Tadeu Zagallo]

(In reply to Saam Barati from comment #5)
> Is the plan here to do what I suggested on Slack yesterday to simplify the
> approach? Or keep it as is?

I'm trying the approach you suggested now, I'll see how it goes.

[Tadeu Zagallo]

Created attachment 401769 [details]
Patch

[Keith Miller]

Comment on attachment 401769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401769&action=review

r- because I think it should be a lambda callback. Otherwise it looks good.

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:178
> +Vector<Operand, maxNumCheckpointTmps> livenessForCheckpoint(const CodeBlock& codeBlock, BytecodeIndex bytecodeIndex)

It feels like this should take a scoped lambda rather than return a vector. Almost every case is just going to forward it to someone else.

[Saam Barati]

Comment on attachment 401769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401769&action=review

> Source/JavaScriptCore/dfg/DFGForAllKills.h:90
>              // No locals can die at a checkpoint.

Is this still definitely the case?

[Keith Miller]

Comment on attachment 401769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401769&action=review

>> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:178
>> +Vector<Operand, maxNumCheckpointTmps> livenessForCheckpoint(const CodeBlock& codeBlock, BytecodeIndex bytecodeIndex)
> 
> It feels like this should take a scoped lambda rather than return a vector. Almost every case is just going to forward it to someone else.

After talking offline, I'm cool with this design if we have an assert we didn't exceed inline capacity before returning the Vector. There may also be a better WTF container type for this, might be worth a look.

[Keith Miller]

(In reply to Keith Miller from comment #10)
> Comment on attachment 401769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401769&action=review
> 
> >> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:178
> >> +Vector<Operand, maxNumCheckpointTmps> livenessForCheckpoint(const CodeBlock& codeBlock, BytecodeIndex bytecodeIndex)
> > 
> > It feels like this should take a scoped lambda rather than return a vector. Almost every case is just going to forward it to someone else.
> 
> After talking offline, I'm cool with this design if we have an assert we
> didn't exceed inline capacity before returning the Vector. There may also be
> a better WTF container type for this, might be worth a look.

For posterity r=me too.

[Tadeu Zagallo]

Created attachment 401931 [details]
Patch

[Keith Miller]

Comment on attachment 401931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401931&action=review

r=me.

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:184
> +#ifndef NDEBUG

Are you sure you need the ifndef? Also, if so, can this be gated on ASSERT_ENABLED?

[Tadeu Zagallo]

Created attachment 401963 [details]
Patch for landing

[EWS]

Committed r263071: <https://trac.webkit.org/changeset/263071>

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

[Saam Barati]

Comment on attachment 401963 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=401963&action=review

> Source/JavaScriptCore/ChangeLog:14
> +

it would be helpful to enumerate here what the bug is and how this fixed it.

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:179
> +Vector<Operand, maxNumCheckpointTmps> livenessForCheckpoint(const CodeBlock& codeBlock, BytecodeIndex bytecodeIndex)

what's the goal here:
- Are we only marking uses for things defined in the same opcode?
- Or are we supposed to mark all uses of a thing?

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:186
> +    auto scopeExit = makeScopeExit([&] {
> +        ASSERT(result.size() <= maxNumCheckpointTmps);
> +    });

is this that important?

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:203
> +            result.append(codeBlock.instructions().at(bytecodeIndex)->as<OpIteratorOpen>().m_iterator);

this is guaranteed to be a local?

> Source/JavaScriptCore/dfg/DFGForAllKills.h:86
> +            const FastBitVector& liveAfter = fullLiveness.getLiveness(after.bytecodeIndex(), LivenessCalculationPoint::BeforeUse);

I think logically we might want AfterUse here, wdyt?

> Source/JavaScriptCore/dfg/DFGForAllKills.h:89
> +                if (checkpointLiveAfter.contains(operand) || (!operand.isTmp() && liveAfter[operand.virtualRegister().offset()]))

You should assert that it's a local if it's not a tmp.

> Source/JavaScriptCore/dfg/DFGGraph.h:909
> +                auto live = livenessForCheckpoint(*codeBlock, codeOriginPtr->bytecodeIndex());
> +                for (Operand operand : live)
> +                    functor(remapOperand(inlineCallFrame, operand));

nit: I don't think you need the variable "live" here, you could just put the call in the for loop

> JSTests/ChangeLog:3
> +        Bytecode liveness should be aware of checkpoints

wrong bug name

[Tadeu Zagallo]

Comment on attachment 401963 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=401963&action=review

>> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:179
>> +Vector<Operand, maxNumCheckpointTmps> livenessForCheckpoint(const CodeBlock& codeBlock, BytecodeIndex bytecodeIndex)
> 
> what's the goal here:
> - Are we only marking uses for things defined in the same opcode?
> - Or are we supposed to mark all uses of a thing?

Right now, the former. The latter is already handled by regular use/def.

>> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:186
>> +    });
> 
> is this that important?

couldn't harm to avoid the malloc, no?

>> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:203
>> +            result.append(codeBlock.instructions().at(bytecodeIndex)->as<OpIteratorOpen>().m_iterator);
> 
> this is guaranteed to be a local?

yes

>> Source/JavaScriptCore/dfg/DFGForAllKills.h:86
>> +            const FastBitVector& liveAfter = fullLiveness.getLiveness(after.bytecodeIndex(), LivenessCalculationPoint::BeforeUse);
> 
> I think logically we might want AfterUse here, wdyt?

I read the code a few times, I'm not sure I see the difference between AfterUse for the `before` bytecode and BeforeUse for the `after` bytecode. I'll read some more.

>> Source/JavaScriptCore/dfg/DFGForAllKills.h:89
>> +                if (checkpointLiveAfter.contains(operand) || (!operand.isTmp() && liveAfter[operand.virtualRegister().offset()]))
> 
> You should assert that it's a local if it's not a tmp.

I'll add a follow up patch

>> JSTests/ChangeLog:3
>> +        Bytecode liveness should be aware of checkpoints
> 
> wrong bug name

oops, my bad

[Saam Barati]

Comment on attachment 401963 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=401963&action=review

>>> Source/JavaScriptCore/dfg/DFGForAllKills.h:86
>>> +            const FastBitVector& liveAfter = fullLiveness.getLiveness(after.bytecodeIndex(), LivenessCalculationPoint::BeforeUse);
>> 
>> I think logically we might want AfterUse here, wdyt?
> 
> I read the code a few times, I'm not sure I see the difference between AfterUse for the `before` bytecode and BeforeUse for the `after` bytecode. I'll read some more.

I'm not 100% sure about this. It just feels weird we're suing live-in to this bytecode after we're already executing in some checkpoints to check live after.

[Saam Barati]

Talking w/ Tadeu on slack, I think a theoretical example would be something like:

```
a -> def loc42
b -> use loc42
c -> use/def nothing
```

and a has 4 checkpoints:
```
a1 -> def loc42
a2 -> use loc42
a3
a4
b  -> use loc42
c
```

For such a thing today, I think we'll say loc42 is killed from a2->a3 since bytecode liveness is giving us live-in to a1, so it doesn't contain loc42

In reality, b is the kill