[webkit-reviews] review denied: [Bug 93401] Remove uses of ClassInfo in StrictEq and CompareEq in the DFG : [Attachment 160506] 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 93401: Remove uses of ClassInfo in StrictEq and CompareEq in the DFG
https://bugs.webkit.org/show_bug.cgi?id=93401

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

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


I buy your changes to the backend, but I don't think that your decision
procedure for choosing when to speculate cell-that-is-not-string is right.  It
will let through values that may be strings.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:773
> +	       if (left.shouldSpeculateCellOrOther() &&
right.shouldSpeculateCell()) {
>		   node.setCanExit(
> -		       !isArraySpeculation(forNode(node.child1()).m_type)
> -		       ||
!isArrayOrOtherSpeculation(forNode(node.child2()).m_type));
> -		   forNode(node.child1()).filter(SpecArray);
> -		   forNode(node.child2()).filter(SpecArray | SpecOther);
> +		       !isCellOrOtherSpeculation(forNode(node.child1()).m_type)

> +		       || !isCellSpeculation(forNode(node.child2()).m_type)
> +		       ||
m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint
()->isStillValid());
> +		   forNode(node.child1()).filter((SpecCell & ~SpecString) |
SpecOther);
> +		   forNode(node.child2()).filter((SpecCell & ~SpecString) |
SpecOther);

I'm not sure I like how this case will be hit if a value can be either an
object or a string (the previous if would not have fired since it only accepts
strings), and then you'll speculate that the cell is not a string.  Can you
change shouldSpeculateCellOrOther to shouldSpeculateNonStringCellOrOther?

Also, if we had proved that the thing is a string, then this will exit, but
your setCanExit() call will say false.	As I've mentioned before, you can fix
this by just doing node.setCanExit(true).  That is the safest thing to do here.