[webkit-reviews] review granted: [Bug 204317] [JSC] DFG terminal's liveness should respect caller's opcodeID : [Attachment 384015] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 204317: [JSC] DFG terminal's liveness should respect caller's opcodeID
https://bugs.webkit.org/show_bug.cgi?id=204317

Attachment 384015: Patch

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




--- Comment #8 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 384015
  --> https://bugs.webkit.org/attachment.cgi?id=384015
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:29
> +	   in DFG, but this is wasting.

"wasting" => "wasteful". Maybe also say "because varargs forwarding phase could
eliminate the allocation."

> Source/JavaScriptCore/ChangeLog:33
> +	   is not profitable, and (2) we need to be careful to make stack
arguments live to allow materialization of arguments objects.

why would (2) not Just Work? It seems more principled to apply this to all
calls and see if there is any bug fallout. I don't see why we should special
case varargs calls.

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:43
> +// <- AfterUse
> +//	  Use by exception handlers

nice!

> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:46
> +    BeforeUse = 0,

is = 0 needed for anything?

> Source/JavaScriptCore/dfg/DFGGraph.h:916
> +	   InlineCallFrame* inlineCallFrame = origin.inlineCallFrame();
> +	   CodeBlock* codeBlock = baselineCodeBlockFor(inlineCallFrame);
> +	   auto instruction =
codeBlock->instructions().at(bytecodeIndex.offset());

nit: if you keep this code, you should put it inside the "if".

However, I think the more robust thing is for all inlined things, we used
BeforeUse.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:94
> +# After calling, calling bytecode is claiming input registers are not used.

are these comments needed?