[webkit-reviews] review denied: [Bug 125026] Implement a one-pass CSE using a hash table : [Attachment 218122] 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: Implement a one-pass CSE using a hash table
https://bugs.webkit.org/show_bug.cgi?id=125026

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

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


> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:133
> +    unsigned hashNode(Node *node)
> +    {
> +	   if (node->op() == JSConstant)
> +	       return (node->constantNumber() << 2) ^ 17;
> +
> +	   if (node->op() == WeakJSConstant)
> +	       return reinterpret_cast<uintptr_t>(node->weakConstant());
> +
> +	   unsigned h = (node->op() << 3) ^ node->arithNodeFlags() ^
node->prediction();
> +	   for (unsigned i = 0; i < 3; i++) {
> +	       Edge edge = node->child(i);
> +	       if (!edge)
> +		   break;
> +
> +	       h <<= 6;
> +	       h ^= (edge.node()->op() << 3)  ^ edge.node()->arithNodeFlags() ^
edge.node()->prediction();
> +	       if (edge.node()->op() == JSConstant)
> +		   h = (h << 4) ^ edge.node()->constantNumber();
> +	   }
> +	   return h;
> +    }
> +
> +    bool nodeEquivalent(Node *a, Node *b)
> +    {
> +	   // Same opcode.
> +	   if (a->op() != b->op())
> +	       return false;
> +
> +	   // Handle Constants.
> +	   if (a->op() == JSConstant)
> +	       return a->constantNumber() == b->constantNumber();
> +
> +	   // Handle Weak Constants.
> +	   if (a->op() == WeakJSConstant)
> +	       return a->weakConstant() == b->weakConstant();
> +
> +	   // Same flags.
> +	   if (a->arithNodeFlags() != b->arithNodeFlags())
> +	       return false;
> +
> +	   // Same operands.
> +	   for (unsigned i = 0; i < 3; i++) {
> +	       Edge edge = b->child(i);
> +	       if (!edge)
> +		   break;
> +
> +	       if (a->child(i) != edge)
> +		   return false;
> +	   }
> +
> +	   // Nodes are identical.
> +	   return true;
> +    }

These feel like they ought to be methods of DFG::Node.

> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:224
> +    bool isPure(Node *node)
> +    {
> +	   switch (node->op()) {
> +	   case Identity:
> +	   case BitAnd:
> +	   case BitOr:
> +	   case BitXor:
> +	   case BitRShift:
> +	   case BitLShift:
> +	   case BitURShift:
> +	   case ArithAdd:
> +	   case ArithSub:
> +	   case ArithNegate:
> +	   case ArithMul:
> +	   case ArithIMul:
> +	   case ArithMod:
> +	   case ArithDiv:
> +	   case ArithAbs:
> +	   case ArithMin:
> +	   case ArithMax:
> +	   case ArithSqrt:
> +	   case ArithSin:
> +	   case ArithCos:
> +	   case StringCharAt:
> +	   case StringCharCodeAt:
> +	   case IsUndefined:
> +	   case IsBoolean:
> +	   case IsNumber:
> +	   case IsString:
> +	   case IsObject:
> +	   case IsFunction:
> +	   case DoubleAsInt32:
> +	   case LogicalNot:
> +	   case SkipTopScope:
> +	   case SkipScope:
> +	   case GetClosureRegisters:
> +	   case GetScope:
> +	   case TypeOf:
> +	   case CompareEqConstant:
> +	   case ValueToInt32:
> +	   case MakeRope:
> +	   case Int52ToDouble:
> +	   case Int52ToValue:
> +	       return true;
> +	   case Int32ToDouble:
> +	   case GetCallee:
> +	   case GetLocal:
> +	   case GetLocalUnlinked:
> +	   case Flush:
> +	       return false;
> +	   case JSConstant:
> +	   case WeakJSConstant:
> +	       return true;
> +	   case GetArrayLength:
> +	   case GetMyScope:
> +	       return false;
> +
> +	   // These nodes are only conditionally pure. Ignore them for now.
> +	   case ValueAdd:
> +	   case CompareLess:
> +	   case CompareLessEq:
> +	   case CompareGreater:
> +	   case CompareGreaterEq:
> +	   case CompareEq:
> +	       return false;
> +	   case GetGlobalVar:
> +	   case GetClosureVar:
> +	   case VarInjectionWatchpoint:
> +	   case PutGlobalVar:
> +	   case PutClosureVar:
> +	   case GetByVal:
> +	   case PutByValDirect:
> +	   case PutByVal:
> +	   case CheckStructure:
> +	   case StructureTransitionWatchpoint:
> +	   case PutStructure:
> +	   case CheckFunction:
> +	   case CheckExecutable:
> +	   case CheckArray:
> +	   case GetIndexedPropertyStorage:
> +	   case GetTypedArrayByteOffset:
> +	   case GetButterfly:
> +	   case GetByOffset:
> +	   case PutByOffset:
> +	   case Phantom:
> +	       return false;
> +	   default:
> +	       return false;
> +	   }
> +    }

You should find a way to use DFGClobberize.h for this.