[webkit-reviews] review denied: [Bug 125026] Refactor pureCSE : [Attachment 218086] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Nadav Rotem <nrotem at apple.com>'s
request for review:
Bug 125026: Refactor pureCSE
https://bugs.webkit.org/show_bug.cgi?id=125026

Attachment 218086: Patch
https://bugs.webkit.org/attachment.cgi?id=218086&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218086&action=review


> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:102
> +    bool nodeEquivalent(Node *A, Node *B)
> +    {
> +	   // Same opcode.
> +	   if (A->op() != B->op())
> +	       return false;
> +
> +	   // Same flags.
> +	   if (A->arithNodeFlags() != B->arithNodeFlags())
> +	       return false;
> +
> +	   // Same operands.
> +	   for (unsigned i = 0; i < 3; i++) {
> +	       Edge E = B->child(i);
> +	       if (!E)
> +		   break;
> +
> +	       if (A->child(i) != E)
> +		   return false;
> +	   }
> +
> +	   // Nodes are identical.
> +	   return true;
> +    }

I think that this needs some naming convention love:

- "A" and "B" are not in line with WebKit variable naming conventions.	We
prefer full words, and we prefer camelCase with the leading letter being lower
case.  We do sometimes use "a" and "b" (not "A" and "B") and that may be
appropriate here.

- The edge should be named "edge" not "E".

> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:112
> +	       // Node can't be equivalent to one of its operands.

I think this comment is wrong.	The point of this early break (return) is to
optimize the run-time of the CSE: we won't search backwards past one of the
nodes that a node depends on.  Without this we would keep searching until the
start of a basic block for any node that doesn't have a replacement.