[webkit-reviews] review granted: [Bug 164108] Optimize SharedArrayBuffer in the DFG+FTL : [Attachment 307396] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 164108: Optimize SharedArrayBuffer in the DFG+FTL
https://bugs.webkit.org/show_bug.cgi?id=164108

Attachment 307396: the patch

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




--- Comment #19 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 307396
  --> https://bugs.webkit.org/attachment.cgi?id=307396
the patch

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

r=me

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:706
> +	       if (jump.isSet())
> +		   m_jumps.append(jump);

This seems like it would usually be a bug in the caller.

> Source/JavaScriptCore/assembler/CPU.h:76
> +inline bool is64Bit()
> +{
> +    return sizeof(void*) == 8;
> +}
> +
> +inline bool is32Bit()
> +{
> +    return !is64Bit();
> +}

Why not just use JSVALUE64 for these?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:555
> +    case AtomicsIsLockFree: {
> +	   if (node->child1().useKind() != Int32Use)
> +	       clobberWorld(node->origin.semantic, clobberLimit);
> +	   forNode(node).setType(SpecBoolInt32);
> +	   break;
> +    }

Maybe it's worth having a constant folding rule here? I could see this often
being used with a constant literal by users of the API.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:943
> +	       fixEdge<KnownInt32Use>(index);

Is this KnownInt32 b/c blessArrayOperation?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3194
> +	   Edge argEdges[2];
> +	   for (unsigned i = numExtraArgs; i--;)

Can you RELEASE_ASSERT here that numExtraArgs <= 2?
Also, it might be worth giving the literal "2" a name, since you use the
literal 2 in a few places.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3296
> +	   // OOPS! need bug.

Please create and link here.

Also, I don't understand why this is needed. Can you explain?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2967
> +	   Edge argEdges[2];
> +	   for (unsigned i = numExtraArgs; i--;)

Ditto about adding an assertion and also perhaps using a name for "2".