AOT: Enable branch merging for checked smi comparisons

AOT: Enable branch merging for checked smi comparisons

This generates better code for smi comparisons in AOT code where the inputs
are not known smis: It avoids materializing the bool-value before branching.

Before:

;; v46 <- CheckedSmiOp:86(==, v35, v77)
;; Branch if StrictCompare:88(!==, v46, v38) goto (8, 9)
0x10e0c2ca3    f6c201                 testb rdx,1
0x10e0c2ca6    0f8526010000           jnz 0x10e0c2dd2
0x10e0c2cac    483bca                 cmpq rcx,rdx
0x10e0c2caf    0f8409000000           jz 0x10e0c2cbe
0x10e0c2cb5    498b5e58               movq rbx,[thr+0x58]   false
0x10e0c2cb9    e904000000             jmp 0x10e0c2cc2
0x10e0c2cbe    498b5e50               movq rbx,[thr+0x50]   true
0x10e0c2cc2    493b5e50               cmpq rbx,[thr+0x50]   true
0x10e0c2cc6    0f8429000000           jz 0x10e0c2cf5  ;; B9
;; B8

===
After:
;; Branch if CheckedSmiComparison:86(!=, v35, v77) goto (8, 9)
0x118b3ce15    f6c201                 testb rdx,1
0x118b3ce18    0f850c010000           jnz 0x118b3cf2a
0x118b3ce1e    483bca                 cmpq rcx,rdx
0x118b3ce21    0f8429000000           jz 0x118b3ce50  ;; B9
;; B8

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/0294f8650d5dab0b786f10fb180e32008a131aae

BUG=#27459

[Florian Schneider, 2016-11-01 12:42:37]

fschneider@google.com changed reviewers:
+ vegorov@google.com

[Florian Schneider, 2016-11-01 13:11:42]

Richards 4.6% speedup

before:
2926
2871
2903
2919
2894

after:
2767
2752
2780
2768
2796

[Vyacheslav Egorov (Google), 2016-11-01 13:19:07]

LGTM 

though I have one question: have you looked at the code size?

if we start pushing checked comparison upwards through Phis that would increase
binary size because slow paths are not shared and thus will be duplicated.

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:2017: if (!current->CanDeoptimize() &&
!current->MayThrow()) {
BranchInstr::CanDeoptimize(...) has some special code for the case when branches
can throw. Maybe we should remove it from there. 

Also I find it confusing that throwing needs deopt info (I always forget why one
needs deopt environments for throwing). So maybe comment should be added here
explaining that.

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:7040: }
If this function is called twice then is_negated_ gets out of sync. Maybe do:

is_negated_ = !is_negated_;

[Florian Schneider, 2016-11-01 15:26:25]

Size change is -20K on Richards. Note that this CL does not hoist branches
through phis. I changed BranchSimplifier to not hoist comparisons that can
throw.

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/flow_graph.c...
runtime/vm/flow_graph.cc:2017: if (!current->CanDeoptimize() &&
!current->MayThrow()) {
On 2016/11/01 13:19:06, Vyacheslav Egorov (Google) wrote:
> BranchInstr::CanDeoptimize(...) has some special code for the case when
branches
> can throw. Maybe we should remove it from there. 
> 
> Also I find it confusing that throwing needs deopt info (I always forget why
one
> needs deopt environments for throwing). So maybe comment should be added here
> explaining that.

I'll add a check if the instruction is inside a try-block: It's try-catch that
needs the environment to emit the call-prologue inside try-blocks in optimized
code. Stacktraces don't need them, and deopt-infos are not written in the
snapshot. We omit inlined frames from stacktraces in AOT compiled code for
snapshot size reasons.

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2466643002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:7040: }
On 2016/11/01 13:19:07, Vyacheslav Egorov (Google) wrote:
> If this function is called twice then is_negated_ gets out of sync. Maybe do:
> 
> is_negated_ = !is_negated_;

Yes, thanks.

Done.

[Florian Schneider, 2016-11-01 16:08:30]

Description was changed from

==========
AOT: Enable branch merging for checked smi comparisons

This generates better code for smi comparisons in AOT code where the inputs
are not known smis: It avoids materializing the bool-value before branching.

Before:

;; v46 <- CheckedSmiOp:86(==, v35, v77)
;; Branch if StrictCompare:88(!==, v46, v38) goto (8, 9)
0x10e0c2ca3    f6c201                 testb rdx,1
0x10e0c2ca6    0f8526010000           jnz 0x10e0c2dd2
0x10e0c2cac    483bca                 cmpq rcx,rdx
0x10e0c2caf    0f8409000000           jz 0x10e0c2cbe
0x10e0c2cb5    498b5e58               movq rbx,[thr+0x58]   false
0x10e0c2cb9    e904000000             jmp 0x10e0c2cc2
0x10e0c2cbe    498b5e50               movq rbx,[thr+0x50]   true
0x10e0c2cc2    493b5e50               cmpq rbx,[thr+0x50]   true
0x10e0c2cc6    0f8429000000           jz 0x10e0c2cf5  ;; B9
;; B8

===
After:
;; Branch if CheckedSmiComparison:86(!=, v35, v77) goto (8, 9)
0x118b3ce15    f6c201                 testb rdx,1
0x118b3ce18    0f850c010000           jnz 0x118b3cf2a
0x118b3ce1e    483bca                 cmpq rcx,rdx
0x118b3ce21    0f8429000000           jz 0x118b3ce50  ;; B9
;; B8
==========

to

==========
AOT: Enable branch merging for checked smi comparisons

This generates better code for smi comparisons in AOT code where the inputs
are not known smis: It avoids materializing the bool-value before branching.

Before:

;; v46 <- CheckedSmiOp:86(==, v35, v77)
;; Branch if StrictCompare:88(!==, v46, v38) goto (8, 9)
0x10e0c2ca3    f6c201                 testb rdx,1
0x10e0c2ca6    0f8526010000           jnz 0x10e0c2dd2
0x10e0c2cac    483bca                 cmpq rcx,rdx
0x10e0c2caf    0f8409000000           jz 0x10e0c2cbe
0x10e0c2cb5    498b5e58               movq rbx,[thr+0x58]   false
0x10e0c2cb9    e904000000             jmp 0x10e0c2cc2
0x10e0c2cbe    498b5e50               movq rbx,[thr+0x50]   true
0x10e0c2cc2    493b5e50               cmpq rbx,[thr+0x50]   true
0x10e0c2cc6    0f8429000000           jz 0x10e0c2cf5  ;; B9
;; B8

===
After:
;; Branch if CheckedSmiComparison:86(!=, v35, v77) goto (8, 9)
0x118b3ce15    f6c201                 testb rdx,1
0x118b3ce18    0f850c010000           jnz 0x118b3cf2a
0x118b3ce1e    483bca                 cmpq rcx,rdx
0x118b3ce21    0f8429000000           jz 0x118b3ce50  ;; B9
;; B8

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0294f8650d5dab0b786f10fb180e32008a131aae
==========

[Florian Schneider, 2016-11-01 16:08:31]

Committed patchset #4 (id:60001) manually as
0294f8650d5dab0b786f10fb180e32008a131aae (presubmit successful).

[Florian Schneider, 2016-11-02 09:17:52]

Description was changed from

==========
AOT: Enable branch merging for checked smi comparisons

This generates better code for smi comparisons in AOT code where the inputs
are not known smis: It avoids materializing the bool-value before branching.

Before:

;; v46 <- CheckedSmiOp:86(==, v35, v77)
;; Branch if StrictCompare:88(!==, v46, v38) goto (8, 9)
0x10e0c2ca3    f6c201                 testb rdx,1
0x10e0c2ca6    0f8526010000           jnz 0x10e0c2dd2
0x10e0c2cac    483bca                 cmpq rcx,rdx
0x10e0c2caf    0f8409000000           jz 0x10e0c2cbe
0x10e0c2cb5    498b5e58               movq rbx,[thr+0x58]   false
0x10e0c2cb9    e904000000             jmp 0x10e0c2cc2
0x10e0c2cbe    498b5e50               movq rbx,[thr+0x50]   true
0x10e0c2cc2    493b5e50               cmpq rbx,[thr+0x50]   true
0x10e0c2cc6    0f8429000000           jz 0x10e0c2cf5  ;; B9
;; B8

===
After:
;; Branch if CheckedSmiComparison:86(!=, v35, v77) goto (8, 9)
0x118b3ce15    f6c201                 testb rdx,1
0x118b3ce18    0f850c010000           jnz 0x118b3cf2a
0x118b3ce1e    483bca                 cmpq rcx,rdx
0x118b3ce21    0f8429000000           jz 0x118b3ce50  ;; B9
;; B8

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0294f8650d5dab0b786f10fb180e32008a131aae
==========

to

==========
AOT: Enable branch merging for checked smi comparisons

This generates better code for smi comparisons in AOT code where the inputs
are not known smis: It avoids materializing the bool-value before branching.

Before:

;; v46 <- CheckedSmiOp:86(==, v35, v77)
;; Branch if StrictCompare:88(!==, v46, v38) goto (8, 9)
0x10e0c2ca3    f6c201                 testb rdx,1
0x10e0c2ca6    0f8526010000           jnz 0x10e0c2dd2
0x10e0c2cac    483bca                 cmpq rcx,rdx
0x10e0c2caf    0f8409000000           jz 0x10e0c2cbe
0x10e0c2cb5    498b5e58               movq rbx,[thr+0x58]   false
0x10e0c2cb9    e904000000             jmp 0x10e0c2cc2
0x10e0c2cbe    498b5e50               movq rbx,[thr+0x50]   true
0x10e0c2cc2    493b5e50               cmpq rbx,[thr+0x50]   true
0x10e0c2cc6    0f8429000000           jz 0x10e0c2cf5  ;; B9
;; B8

===
After:
;; Branch if CheckedSmiComparison:86(!=, v35, v77) goto (8, 9)
0x118b3ce15    f6c201                 testb rdx,1
0x118b3ce18    0f850c010000           jnz 0x118b3cf2a
0x118b3ce1e    483bca                 cmpq rcx,rdx
0x118b3ce21    0f8429000000           jz 0x118b3ce50  ;; B9
;; B8

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0294f8650d5dab0b786f10fb180e32008a131aae

BUG=#27459
==========