...
<rdar://problem/63416838>
Created attachment 401697 [details] Patch
Created attachment 401698 [details] Patch
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?
Is the plan here to do what I suggested on Slack yesterday to simplify the approach? Or keep it as is?
(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.
Created attachment 401769 [details] Patch
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.
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?
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.
(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.
Created attachment 401931 [details] Patch
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?
Created attachment 401963 [details] Patch for landing
Committed r263071: <https://trac.webkit.org/changeset/263071> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401963 [details].
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
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
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.
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