[Webkit-unassigned] [Bug 142966] Optimize typed array access with a masked index

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=142966

--- Comment #4 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 249236
  --> https://bugs.webkit.org/attachment.cgi?id=249236
Optimize away bounds checks when given masked typed array indexes

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

Looks pretty good so far, but please move the helper to a more appropriate place.  And please don't remove the old flow-sensitive CheckInBounds elimination.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2301
> +bool indexInBounds(Node* indexNode, int32_t length);

This declaration should be anywhere but here.  It's wrong to declare a method in X.h and define it in Y.h - at the very least, the header in which it's declared should correspond to the compilation module in which it is defined.  It's also wrong to make declarations in Inlines.h files; those are reserved for definitions of inline functions.  This isn't an inline function, and it's not a definition.

We often put such universal helpers in Graph.  You can continue this pattern.  Just add a method to Graph called indexInBounds().

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:46
> +// Pattern match an array index expression returning true if it is known to be within
> +// bounds otherwise false. This matches a constant index, or a masked index with an
> +// optional constant offset, and optionally right shifted.
> +bool indexInBounds(Node* indexNode, int32_t length)

See above.  This definition is also in the wrong place.  It's an anti-pattern to define helpers inside the compilation module for a phase.  (We do make this mistake sometimes, but I'd really rather we stop doing it.)

We usually put such helpers in Graph.h/cpp.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:269
> -                JSValue left = m_state.forNode(node->child1()).value();
> +                // The length is obtained in strength reduction.
>                  JSValue right = m_state.forNode(node->child2()).value();
> -                if (left && right && left.isInt32() && right.isInt32()
> -                    && static_cast<uint32_t>(left.asInt32()) < static_cast<uint32_t>(right.asInt32())) {
> +                if (!right || !right.isInt32())
> +                    break;
> +                // Array length is known.
> +                int32_t length = right.asInt32();
> +                if (indexInBounds(node->child1().node(), length)) {

It appears that you're relying on the left child being folded to a constant, rather than it being proved constant.  There is a small difference, but it's a difference we prefer to deal with head-on.  The constant folder can theoretically produce flow-sensitive constants.  For example, a node may be constant only along some control flow path.  In that case, this will miss CheckInBounds where the index was such a partial constant.

Can you bring back the old folding rule, that did handle the flow-sensitive case?  I believe it's just a matter of not removing the old "if (left && right && left.isInt32() && right.isInt32() && static_cast<uint32_t>(left.asInt32()) < static_cast<uint32_t>(right.asInt32())..." thing.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150323/e90dec4e/attachment-0002.html>