[webkit-reviews] review denied: [Bug 221438] [JSC] Atomics.store in DFG / FTL should return ToNumber(input) value : [Attachment 419341] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 221438: [JSC] Atomics.store in DFG / FTL should return ToNumber(input)
value
https://bugs.webkit.org/show_bug.cgi?id=221438

Attachment 419341: Patch

https://bugs.webkit.org/attachment.cgi?id=419341&action=review




--- Comment #3 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 419341
  --> https://bugs.webkit.org/attachment.cgi?id=419341
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419341&action=review

I think you should at least rationalize toIntegerOrInfinity - usually when we
say "toFooOrBlah", we mean "convert to foo or return blah", and here you're
returning zero.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2353
> +	   if (node->op() == AtomicsStore) {
> +	       switch (node->arrayMode().type()) {
> +	       case Array::Generic:
> +		   clobberWorld();
> +		   makeHeapTopForNode(node);
> +		   break;
> +	       default: {
> +		   switch (node->result()) {
> +		   case NodeResultJS:
> +		       setNonCellTypeForNode(node, SpecInt32Only);
> +		       break;
> +		   case NodeResultInt52:
> +		       setNonCellTypeForNode(node, SpecInt52Any);
> +		       break;
> +		   case NodeResultDouble:
> +		       setNonCellTypeForNode(node, SpecFullDouble);
> +		       break;
> +		   default:
> +		       DFG_CRASH(m_graph, node, "Bad result type");
> +		       break;
> +		   }
> +		   break;
> +	       }
> +	       }
> +	       break;
> +	   }

It's not clear why the switch below this wouldn't handle this properly.  I
think that this is worth a good comment.  It's also a bit weird that we're
casing on result type rather than the operand type.  Usually, we use the
operand's useKind as the criterion we switch on - and from reading the fixup
code, it seems like the result type is completely determined by the stored
value operand useKind.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:19659
> +    LValue toIntegerOrInfinity(LValue doubleValue)
> +    {
> +	   return m_out.select(m_out.doubleNotEqualAndOrdered(doubleValue,
m_out.doubleZero), m_out.doubleTrunc(doubleValue), m_out.doubleZero);
> +    }

This returns zero in the else case, not infinity.