[webkit-reviews] review denied: [Bug 135593] Using +s to convert a string to a number badly confuses the DFG : [Attachment 236052] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 135593: Using +s to convert a string to a number badly confuses the DFG
https://bugs.webkit.org/show_bug.cgi?id=135593

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

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


Almost there

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:309
> +    case ValueToNumber: {

Can we call it ToNumber?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:318
> +	   if (child && child.isInt32()) {
> +	       setConstant(node, child);
> +	       break;
> +	   }
> +	   if (child && child.isNumber()) {
> +	       setConstant(node, jsDoubleNumber(child.asNumber()));
> +	       break;
> +	   }

I feel like this should just be:

if (child && child.isNumber()) {
    setConstant(node, child);
    break;
}

> Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:362
> +	       node->child1()->mergeFlags(flags);

I would mask off NodeBytecodeUsesAsOther, as in:
node->child1()->mergeFlags(flags & ~NodeNytecodeUsesAsOther)

> Source/JavaScriptCore/dfg/DFGClobberize.h:351
> +	   read(MiscFields);

It reads world.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1153
> +	   case ValueToNumber:

This needs to have the identity stuff.	And probably some stuff to use
BooleanToNumber.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:479
> +	       changed |= setPrediction(SpecFullNumber);

This needs to be more granular.  If the input is already an int or already a
number then you should preserve the original input's type.  Literally you want
something like:

if (node->child1()->prediction() & SpecCell)
    changed |= setPrediction(SpecFullNumber);
else {
    SpeculatedType type = node->child1()->prediction();
    if (type & SpecOther) {
	type &= ~SpecOther;
	type |= SpecInt32 | SpecDoubleNaN;
    }
    if (type & SpecBoolean) {
	type &= ~SpecBoolean; 
	type |= SpecInt32;
    }
    mergePredicstion(type);
}