Fix for running with --throw_on_javascript_int_overflow: recognize pattern (a << b) & mask and test…

Fix for running with --throw_on_javascript_int_overflow: recognize pattern (a << b) & mask and test for overflow only after the mask.

R=fschneider@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26122

[srdjan, 2013-08-12 21:51:52]

Seems to have no performance impact except 18% gain on SHA256, x64.

[Florian Schneider, 2013-08-13 14:45:23]

I think we need a more generic approach because I don't think adding recognized
patterns in the parser will scale: It seems there are too many cases that would
need attention.

Programmers would not be able to store a frequently used mask in a variable:

var m = 0xffffffff;

a << 1 & m;
b << 1 & m;
// etc.

We should support the case where the definition of the mask and the uses can be
arbitrary far apart. Also, how about

(x * 2) & 0xffffffff

vs. 

(x << 1) & 0xffffffff

The first would throw, the second won't. Possibly also the same issue with
bitwise-not: (~x & mask)

[Florian Schneider, 2013-08-13 15:29:52]

lgtm.

As discussed off-line, before adding more patterns like this we should consider
a more generic approach.

https://codereview.chromium.org/22640019/diff/52001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/22640019/diff/52001/runtime/vm/ast.h#newcode613
runtime/vm/ast.h:613: int64_t mask32_;
I think it would be cleaner to have a separate AST node. It would also avoid
adding 64 bit to all the binary op nodes.

[srdjan, 2013-08-13 18:44:59]

PTAL

https://codereview.chromium.org/22640019/diff/52001/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/22640019/diff/52001/runtime/vm/ast.h#newcode613
runtime/vm/ast.h:613: int64_t mask32_;
On 2013/08/13 15:29:52, Florian Schneider wrote:
> I think it would be cleaner to have a separate AST node. It would also avoid
> adding 64 bit to all the binary op nodes.

Done.

[Florian Schneider, 2013-08-14 08:42:47]

lgtm

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1096: void
ValueGraphVisitor::VisitBinaryOpWithMask32Node(
You can just have the Visit method on the EffectGraphVisitor if they perform the
same work because ValueGraphVisitor inherits from EffectGraphVisitor.

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:422: virtual void
VisitBinaryOpWithMask32Node(BinaryOpWithMask32Node* node);
No need to override this method if it does the same as
EffectGraphVisitor::VisitVisitBinaryOpWithMask32Node.

[srdjan, 2013-08-14 14:29:05]

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1096: void
ValueGraphVisitor::VisitBinaryOpWithMask32Node(
On 2013/08/14 08:42:47, Florian Schneider wrote:
> You can just have the Visit method on the EffectGraphVisitor if they perform
the
> same work because ValueGraphVisitor inherits from EffectGraphVisitor.

Done.

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/22640019/diff/123001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:422: virtual void
VisitBinaryOpWithMask32Node(BinaryOpWithMask32Node* node);
On 2013/08/14 08:42:47, Florian Schneider wrote:
> No need to override this method if it does the same as
> EffectGraphVisitor::VisitVisitBinaryOpWithMask32Node.

Done.

[srdjan, 2013-08-14 15:12:49]

Committed patchset #15 manually as r26122 (presubmit successful).