[webkit-reviews] review granted: [Bug 229353] Allow WASM to use up to 4GB : [Attachment 440966] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted  review:
Bug 229353: Allow WASM to use up to 4GB
https://bugs.webkit.org/show_bug.cgi?id=229353

Attachment 440966: Patch

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




--- Comment #51 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 440966
  --> https://bugs.webkit.org/attachment.cgi?id=440966
Patch

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

r=me. When landing, please ensure the following things too.

1. Land tests that exercise new paths. The test we discussed on slack should be
included.
2. Make sure all JSC tests are passed. Currently, some tests are failing.

> Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp:1312
> +		   case CheckInBoundsInt52: {

We should not enable range optimization for Int52 since currently range
optimization is based on Int32.

> Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp:1387
> +	   case CheckInBoundsInt52: {

We should not enable range optimization for Int52 since currently range
optimization is based on Int32.

> Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp:1504
> +	   case GetTypedArrayLengthAsInt52:

We should not enable range optimization for Int52 since currently range
optimization is based on Int32.

> Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:107
> +		       Node* length = m_insertionSet.insertNode(
> +			   m_nodeIndex, SpecInt52Any,
GetTypedArrayLengthAsInt52, m_node->origin,
> +			   OpInfo(m_node->arrayMode().asWord()), base,
storage);
> +		       m_graph.varArgChild(m_node, 4) = Edge(length,
Int52RepUse);

Just changing this is not correct. It is modifying PutByVal's 5th child. So all
the code using PutByVal / PutByValDirect needs to be revisited and ensured that
length can be Int52.
I've checked briefly and FTLLowerDFGToB3 is not handling this change. Please
review all code using PutByVal / PutByValDirect to ensure that this change is
correctly accepted.

   6012 		    m_out.branch(
   6013 			m_out.aboveOrEqual(index, lowInt32(child5)),
   6014 			unsure(isOutOfBounds), unsure(isInBounds));

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5149
> +	   speculate(
> +	       OutOfBounds, noValue(), nullptr,
> +	       m_out.aboveOrEqual(lowStrictInt52(m_node->child1()),
lowStrictInt52(m_node->child2())));

I think using lowStrictInt52 for child1 is not correct because I don't see any
conversion from Int32 to Int52. (And I think it should be Int32 right now since
we do not have GetByVal etc. with Int52 index).
I think it is Int32. And if it is Int32, please remember that we need to have
sign-extension after lowInt32 to extend it to 64bit.
And can you add a test to exercise this path?