[webkit-reviews] review denied: [Bug 93884] Change behavior of MasqueradesAsUndefined to better accommodate DFG changes : [Attachment 160247] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied  review:
Bug 93884: Change behavior of MasqueradesAsUndefined to better accommodate DFG
changes
https://bugs.webkit.org/show_bug.cgi?id=93884

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

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


> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:788
> -		       ||
!isFinalObjectOrOtherSpeculation(forNode(node.child2()).m_type));
> +		       ||
!isFinalObjectOrOtherSpeculation(forNode(node.child2()).m_type)
> +		       ||
m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint
()->isStillValid());
>		   forNode(node.child1()).filter(SpecFinalObject);
>		   forNode(node.child2()).filter(SpecFinalObject | SpecOther);
>		   break;
>	       } else if (right.shouldSpeculateFinalObject() &&
left.shouldSpeculateFinalObjectOrOther()) {
>		   node.setCanExit(
>		      
!isFinalObjectOrOtherSpeculation(forNode(node.child1()).m_type)
> -		       ||
!isFinalObjectSpeculation(forNode(node.child2()).m_type));
> +		       ||
!isFinalObjectSpeculation(forNode(node.child2()).m_type)
> +		       ||
m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint
()->isStillValid());
>		   forNode(node.child1()).filter(SpecFinalObject | SpecOther);
>		   forNode(node.child2()).filter(SpecFinalObject);
>		   break;
>	       } else if (left.shouldSpeculateArray() &&
right.shouldSpeculateArrayOrOther()) {
>		   node.setCanExit(
>		       !isArraySpeculation(forNode(node.child1()).m_type)
> -		       ||
!isArrayOrOtherSpeculation(forNode(node.child2()).m_type));
> +		       ||
!isArrayOrOtherSpeculation(forNode(node.child2()).m_type)
> +		       ||
m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint
()->isStillValid());
>		   forNode(node.child1()).filter(SpecArray);
>		   forNode(node.child2()).filter(SpecArray | SpecOther);
>		   break;
>	       } else if (right.shouldSpeculateArray() &&
left.shouldSpeculateArrayOrOther()) {
>		   node.setCanExit(
>		      
!isArrayOrOtherSpeculation(forNode(node.child1()).m_type)
> -		       || !isArraySpeculation(forNode(node.child2()).m_type));
> +		       || !isArraySpeculation(forNode(node.child2()).m_type)
> +		       ||
m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint
()->isStillValid());

Why did you change this?  Your patch doesn't change any of the backend code
that corresponds to this!

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:796
> +		   node.setCanExit(
> +		       !isAnySpeculation(forNode(node.child1()).m_type)
> +		       || !isAnySpeculation(forNode(node.child2()).m_type)
> +		       ||
m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint
()->isStillValid());

isAnySpeculation returns true all the time.  So this code is very strange.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:798
> +		   forNode(node.child1()).filter(SpecTop);
> +		   forNode(node.child2()).filter(SpecTop);

Please remove this; filtering on TOP does nothing.