[webkit-reviews] review denied: [Bug 133660] [ftlopt] Call and Construct DFG nodes aren't always safe to execute : [Attachment 232879] Split Call and Construct DFG nodes into NativeCall and NativeConstruct

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 133660: [ftlopt] Call and Construct DFG nodes aren't always safe to execute
https://bugs.webkit.org/show_bug.cgi?id=133660

Attachment 232879: Split Call and Construct DFG nodes into NativeCall and
NativeConstruct 
https://bugs.webkit.org/attachment.cgi?id=232879&action=review

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


> Source/JavaScriptCore/dfg/DFGSafeToExecute.h:261
> +	   return false; // TODO: add a check for already checked. 

Can you file a bugzilla bug for this and reference it here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:644
> -    if (node->op() != Call)
> -	   ASSERT(node->op() == Construct);
> +    bool isCall = node->op() == Call || node->op() == NativeCall;
> +    if (!isCall)
> +	   ASSERT(node->op() == Construct || node->op() == NativeConstruct);

Why does this check for NativeCall/NativeConstruct?  The DFG backend shouldn't
see them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4131
> +    case NativeCall:
> +    case NativeConstruct:

Seems like these cases should be RELEASE_ASSERT_NOT_REACHED().

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:631
> -    if (node->op() != Call)
> -	   RELEASE_ASSERT(node->op() == Construct);
> +
> +    bool isCall = node->op() == Call || node->op() == NativeCall;
> +    if (!isCall)
> +	   RELEASE_ASSERT(node->op() == Construct || node->op() ==
NativeConstruct);

Why does this check for NativeCall/NativeConstruct?  The DFG backend shouldn't
see them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4163
> +    case NativeCall:
> +    case NativeConstruct:

Seems like these cases should be RELEASE_ASSERT_NOT_REACHED().