Generate some intrinsics using our IR.

Generate some intrinsics using our IR.

[Florian Schneider, 2014-08-28 16:00:59]

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

[zra, 2014-08-28 17:40:07]

zra@google.com changed reviewers:
+ zra@google.com

[zra, 2014-08-28 17:40:07]

https://codereview.chromium.org/513213002/diff/40001/runtime/vm/constants_arm...
File runtime/vm/constants_arm64.h (right):

https://codereview.chromium.org/513213002/diff/40001/runtime/vm/constants_arm...
runtime/vm/constants_arm64.h:30: kLastFreeCpuRegister = 15,
When writing the port, I checked that the allocator always marks IP0, IP1 and
SPREG as unavailable, so I don't think this change is necessary. Instead, I'd
recommend fixing the bug in the parallel move resolver. Otherwise, I believe the
allocator will never be able to use R19-26, which is probably not what we want.

[Florian Schneider, 2014-08-29 10:30:51]

https://codereview.chromium.org/513213002/diff/40001/runtime/vm/constants_arm...
File runtime/vm/constants_arm64.h (right):

https://codereview.chromium.org/513213002/diff/40001/runtime/vm/constants_arm...
runtime/vm/constants_arm64.h:30: kLastFreeCpuRegister = 15,
On 2014/08/28 17:40:07, zra wrote:
> When writing the port, I checked that the allocator always marks IP0, IP1 and
> SPREG as unavailable, so I don't think this change is necessary. Instead, I'd
> recommend fixing the bug in the parallel move resolver. Otherwise, I believe
the
> allocator will never be able to use R19-26, which is probably not what we
want.

This fix is temporary for now. Ideally, we want to support an arbitrary set of
allocatable registers and not just a continuous range. Right now, there is a lot
of inconsistency. e.g. CTX and PP are inside the range [kFirstFreeCpuRegister,
kLastFreeCpuRegister] on some platforms (x64), and outside that range for others
(ARM64)

Ideally, we want to support an arbitrary set of allocatable registers, and not
just a continuous range.

[Florian Schneider, 2014-09-02 14:34:38]

Updated with more intrinsics ported to the intrinsic graph builder and support
for allocation (slow-path) in intrinsic code.

[srdjan, 2014-09-02 18:48:21]

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_al...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:1790: split_block->loop() != NULL) {
Add parentheses

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:732: if (intrinsic_mode()) return
&intrinsic_deopt_label_;
This feels fragile as we must guaranteed that the lifetime of FlowGraphCompiler
exceeds the lifetime of the new 'owner' of the  intrinsic_deopt_label_.

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:868: // "Deoptimization" from intrinsic
continues here.
We do not deoptimize, this is actually the slow-path, isn't it?

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:64:
assembler()->set_allow_constant_pool(false);
Could you please remind me why we cannot have constants store in constant_pool
inside intrinsic code?

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:940: Location::SameAsFirstInput(),
Why this change?

[zra, 2014-09-02 18:55:07]

Could we split this up into 3 CLs? (Not necessarily in this order)

1. Fix arm64 bugs
2. Fix parallel move resolver bugs.
3. Generate intrinsics with IR.

[Florian Schneider, 2014-09-02 21:03:20]

Sorry for the size - mostly because it affects all 5 platforms, and there are a
lot of inconsistencies, especially in FlowGraphCompiler.

The meat of this CL is really intrinsifier.cc. The abstractions for building the
IR graphs are still pretty basic. I plan to introduce more helpers for the
future (e.g. easily support control flow, right now, all graph intrinsics are
still a single basic block)

The rest is small fixes to make it work on all platforms and pretty mechanically
repeated.

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:732: if (intrinsic_mode()) return
&intrinsic_deopt_label_;
On 2014/09/02 18:48:21, srdjan wrote:
> This feels fragile as we must guaranteed that the lifetime of
FlowGraphCompiler
> exceeds the lifetime of the new 'owner' of the  intrinsic_deopt_label_.

Can you elaborate? There is only one instance of FlowGraphCompiler. It is either
in intrinsic mode, or in normal mode. Intrinsic mode is active before generating
the normal body (slow-path) of the function. When it is active, all
deopt-branches go to this label instead of a real deopt-stub.

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:868: // "Deoptimization" from intrinsic
continues here.
On 2014/09/02 18:48:21, srdjan wrote:
> We do not deoptimize, this is actually the slow-path, isn't it?

That's why it's in quotes. Intrinsic code uses the same IL instructions as the
optimizing compiler. Check instructions that deoptimize in regular optimized
code just branch to here.

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:64:
assembler()->set_allow_constant_pool(false);
On 2014/09/02 18:48:21, srdjan wrote:
> Could you please remind me why we cannot have constants store in constant_pool
> inside intrinsic code?

There is no normal function prologue and therefore, the constant pool register
is not set up in intrinsic code. Only in normal function do that.

Right now, there is no assertion that we actually follow this. Using the
constant pool register in intrinsic code would just cause a crash right now.

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:940: Location::SameAsFirstInput(),
On 2014/09/02 18:48:21, srdjan wrote:
> Why this change?

Not strictly necessary, but reduces number of registers used in the code, so
that spilling in not necessary for external array access on ia32.

[Florian Schneider, 2014-09-02 21:05:46]

On 2014/09/02 18:55:07, zra wrote:
> Could we split this up into 3 CLs? (Not necessarily in this order)
> 
> 1. Fix arm64 bugs
> 2. Fix parallel move resolver bugs.
> 3. Generate intrinsics with IR.

I'm afraid this won't make it much smaller. The ARM64 issues I fixed are all in
2 files: flow_graph_compiler_arm64.cc and constants_arm64.h.

[zra, 2014-09-02 21:09:51]

On 2014/09/02 21:05:46, Florian Schneider wrote:
> On 2014/09/02 18:55:07, zra wrote:
> > Could we split this up into 3 CLs? (Not necessarily in this order)
> > 
> > 1. Fix arm64 bugs
> > 2. Fix parallel move resolver bugs.
> > 3. Generate intrinsics with IR.
> 
> I'm afraid this won't make it much smaller. The ARM64 issues I fixed are all
in
> 2 files: flow_graph_compiler_arm64.cc and constants_arm64.h.

I'd still encourage making this change separately so I can review it quickly and
we can get the fix in, but since it isn't really super urgent, I'll leave it up
to you and let srdjan and slava chime in if they want.

[srdjan, 2014-09-02 21:32:17]

On 2014/09/02 21:09:51, zra wrote:
> On 2014/09/02 21:05:46, Florian Schneider wrote:
> > On 2014/09/02 18:55:07, zra wrote:
> > > Could we split this up into 3 CLs? (Not necessarily in this order)
> > > 
> > > 1. Fix arm64 bugs
> > > 2. Fix parallel move resolver bugs.
> > > 3. Generate intrinsics with IR.
> > 
> > I'm afraid this won't make it much smaller. The ARM64 issues I fixed are all
> in
> > 2 files: flow_graph_compiler_arm64.cc and constants_arm64.h.
> 
> I'd still encourage making this change separately so I can review it quickly
and
> we can get the fix in, but since it isn't really super urgent, I'll leave it
up
> to you and let srdjan and slava chime in if they want.

I vote for splitting it up.
And thanks for all the work in simplifying the system!

[Florian Schneider, 2014-09-03 14:54:55]

On 2014/09/02 21:32:17, srdjan wrote:
> On 2014/09/02 21:09:51, zra wrote:
> > On 2014/09/02 21:05:46, Florian Schneider wrote:
> > > On 2014/09/02 18:55:07, zra wrote:
> > > > Could we split this up into 3 CLs? (Not necessarily in this order)
> > > > 
> > > > 1. Fix arm64 bugs
> > > > 2. Fix parallel move resolver bugs.
> > > > 3. Generate intrinsics with IR.
> > > 
> > > I'm afraid this won't make it much smaller. The ARM64 issues I fixed are
all
> > in
> > > 2 files: flow_graph_compiler_arm64.cc and constants_arm64.h.
> > 
> > I'd still encourage making this change separately so I can review it quickly
> and
> > we can get the fix in, but since it isn't really super urgent, I'll leave it
> up
> > to you and let srdjan and slava chime in if they want.
> 
> I vote for splitting it up.
> And thanks for all the work in simplifying the system!

Ok, I try to split it up as good as possible.

[Florian Schneider, 2014-09-05 12:17:34]

New version with the changes to FlowGraphCompiler factored out.

Added support for allocation (with slow-path). Float64Array.[] and []= is now
implemented in IR.

[zra, 2014-09-05 20:26:52]

Regarding loading null true and false from the pool: I think it would also be
okay to add TODOs to the Assemblers and fix at the same time as converting
intrinsics for which it would matter.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:492: if (!allow_constant_pool()) return false;
Please use curly braces.

Also, Null, True, and False should be loaded from the pool even if this flag is
false since they are in every constant pool.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:514: if (!allow_constant_pool()) return false;
Please use curly braces.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:152: intptr_t call_start = buffer_.GetPosition();
ASSERT that the constant pool is enabled, o/w this won't really be patchable.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2372: intptr_t call_start = buffer_.GetPosition();
Assert that the constant pool is enabled, o/w this won't really be patchable.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2593: if (!allow_constant_pool()) return false;
Please use curly braces.

Also, Null, True, and False should be loaded from the pool even if this flag is
false since they are in every constant pool.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2678: if (!allow_constant_pool()) return false;
Please use curly braces.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1790: split_block->loop() != NULL) {
Add parens.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:732: if (intrinsic_mode()) return
&intrinsic_deopt_label_;
Please use curly braces.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:98: 
extra space.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:12: #include "vm/flow_graph.h"
Maybe alphabetize includes.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:114: // as they don't move, but restrict to smi and
VM objects for now.
The Assemblers for each arch know which VM objects are always available in the
constant pool, and for which you don't need the pool for the current function,
so long as a Dart frame has been entered at some point before entering an
intrinsic (we've already made sure that's the case). The flag for
enabling/disabling constant pool access in the Assemblers should defer to that
logic and possibly be renamed to reflect this. Otherwise, e.g. loading
Bool::True and Bool::False in intrinsics on arm64 will be as many as four
instructions each, which we'd probably like to avoid, and code size on x64 will
grow due to large immediate loads.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:178: if (!CanIntrinsify(function)) return;
Please add curly braces.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:180: if (GraphIntrinsify(parsed_function, compiler))
return;
Here, too.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:234: ReturnInstr* instr = new ReturnInstr(-1,  // No
token position.
Scanner::kNoSourcePos ?

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:263: -1));  // no token pos
Scanner::kNoSourcePos ?

period after comment.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:269: true));  // immutable
period after comment.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:290: Instance::ElementSizeFor(kArrayCid),  // index
scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:293: -1));  // no token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:317: 1,  // index scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:320: -1));  // no token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:342: 1,  // index scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:345: -1));  // no token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:365: -1));  // no token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:372: 1,  // index scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:375: -1));  // no token pos.
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:398: -1));  // no token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:407: 1,  // index scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:410: -1));  // no token pos.
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:444: -1));
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:457: 8,  // index scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:460: -1));  // no token pos.
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:484: 8,  // index scale
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:487: -1));  // no token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:506: -1));  // No token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:548: -1));  // No token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:553: -1));  // No token pos
ditto.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier.h
File runtime/vm/intrinsifier.h (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.h:24: // Try to intrinsify 'function'. Returns true if
the function intrinsified
Maybe update comments.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:25: intptr_t Intrinsifier::ParameterSlotFromSp()
{ return -1; }
Maybe add a constant to stack_frame_*.h

[Florian Schneider, 2014-09-08 11:12:40]

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:492: if (!allow_constant_pool()) return false;
On 2014/09/05 20:26:49, zra wrote:
> Please use curly braces.
> 
> Also, Null, True, and False should be loaded from the pool even if this flag
is
> false since they are in every constant pool.

Ok, I made those objects to be loaded from the constant pool

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.cc:514: if (!allow_constant_pool()) return false;
On 2014/09/05 20:26:49, zra wrote:
> Please use curly braces.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:152: intptr_t call_start = buffer_.GetPosition();
On 2014/09/05 20:26:49, zra wrote:
> ASSERT that the constant pool is enabled, o/w this won't really be patchable.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2372: intptr_t call_start = buffer_.GetPosition();
On 2014/09/05 20:26:49, zra wrote:
> Assert that the constant pool is enabled, o/w this won't really be patchable.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2593: if (!allow_constant_pool()) return false;
On 2014/09/05 20:26:49, zra wrote:
> Please use curly braces.
> 
> Also, Null, True, and False should be loaded from the pool even if this flag
is
> false since they are in every constant pool.
> 

What about the case of an intrinsified function directly invoked from C++? - it
won't have a constant pool register set up from the caller.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2678: if (!allow_constant_pool()) return false;
On 2014/09/05 20:26:50, zra wrote:
> Please use curly braces.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1790: split_block->loop() != NULL) {
On 2014/09/05 20:26:50, zra wrote:
> Add parens.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:732: if (intrinsic_mode()) return
&intrinsic_deopt_label_;
On 2014/09/05 20:26:50, zra wrote:
> Please use curly braces.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:98: 
On 2014/09/05 20:26:50, zra wrote:
> extra space.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:114: // as they don't move, but restrict to smi and
VM objects for now.
On 2014/09/05 20:26:51, zra wrote:
> The Assemblers for each arch know which VM objects are always available in the
> constant pool, and for which you don't need the pool for the current function,
> so long as a Dart frame has been entered at some point before entering an
> intrinsic (we've already made sure that's the case). The flag for
> enabling/disabling constant pool access in the Assemblers should defer to that
> logic and possibly be renamed to reflect this. Otherwise, e.g. loading
> Bool::True and Bool::False in intrinsics on arm64 will be as many as four
> instructions each, which we'd probably like to avoid, and code size on x64
will
> grow due to large immediate loads.
> 

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:178: if (!CanIntrinsify(function)) return;
On 2014/09/05 20:26:50, zra wrote:
> Please add curly braces.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:180: if (GraphIntrinsify(parsed_function, compiler))
return;
On 2014/09/05 20:26:51, zra wrote:
> Here, too.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:234: ReturnInstr* instr = new ReturnInstr(-1,  // No
token position.
On 2014/09/05 20:26:51, zra wrote:
> Scanner::kNoSourcePos ?

Replaced with function.token_pos()

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier.h
File runtime/vm/intrinsifier.h (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.h:24: // Try to intrinsify 'function'. Returns true if
the function intrinsified
On 2014/09/05 20:26:52, zra wrote:
> Maybe update comments.

Done.

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:25: intptr_t Intrinsifier::ParameterSlotFromSp()
{ return -1; }
On 2014/09/05 20:26:52, zra wrote:
> Maybe add a constant to stack_frame_*.h

This is specific for intrinsics, so it seems not useful for other places.

[Florian Schneider, 2014-09-08 11:13:56]

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/513213002/diff/120001/runtime/vm/assembler_x6...
runtime/vm/assembler_x64.cc:2593: if (!allow_constant_pool()) return false;
On 2014/09/08 11:12:39, Florian Schneider wrote:
> On 2014/09/05 20:26:49, zra wrote:
> > Please use curly braces.
> > 
> > Also, Null, True, and False should be loaded from the pool even if this flag
> is
> > false since they are in every constant pool.
> > 
> 
> What about the case of an intrinsified function directly invoked from C++? -
it
> won't have a constant pool register set up from the caller.

Never mind. For the set of VM objects it still works fine since PP is set up in
the invocation stub.

[zra, 2014-09-08 15:52:01]

lgtm, but I'd guess srdjan and slava would also like a chance to make comments.

Thanks for making this change. This will make doing the next port a lot simpler.

[srdjan, 2014-09-08 17:34:07]

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:732: if (intrinsic_mode()) return
&intrinsic_deopt_label_;
On 2014/09/02 21:03:20, Florian Schneider wrote:
> On 2014/09/02 18:48:21, srdjan wrote:
> > This feels fragile as we must guaranteed that the lifetime of
> FlowGraphCompiler
> > exceeds the lifetime of the new 'owner' of the  intrinsic_deopt_label_.
> 
> Can you elaborate? There is only one instance of FlowGraphCompiler. It is
either
> in intrinsic mode, or in normal mode. Intrinsic mode is active before
generating
> the normal body (slow-path) of the function. When it is active, all
> deopt-branches go to this label instead of a real deopt-stub.

I was mainly worried that the lifetime of a FlowGraphCompiler instance may be
shorter than the use of the label. Looking at the code again, my worry is
probably unnecessary

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:630: ASSERT(param->base_reg() == FPREG ||
param->base_reg() == SPREG);
Add parentheses.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.h (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.h:60: bool intrinsics_mode = false);
s/intrinsics_mode/intrinsic_mode/

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.h:328: bool intrinsic_mode_;
const

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:870: // "Deoptimization" from intrinsic
continues here.
Maybe add comment describing why deoptimization is in quotes (deoptimization
mechanism for redirecting to slow path).

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:628: bool intrinsic_mode_;
since bool, maybe rename to is_intrinsic_mode?

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1912: bool is_intrinsic_;
const (also for result_)

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:96: FlowGraph* graph) {
Maybe briefly document why 'graph' is not the same as compiler->flow_graph().
You should stress the fact that none of FlowGraphCompiler operation used should
rely on its own FLowGraph. Ideally we would pass something else than
FlowGraphCompiler, but that would require major restructuring, I think.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:122: FlowGraphBuilder
builder(const_cast<ParsedFunction*>(&parsed_function),
I feel this is dangerous (removing const-ness from const ParsedFunction&
argument) Can you restructure code so that parsed_function that is passed in is
not const &?

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:125: -1);  // No OSR id.
Use Isolate::kNoDeoptId.

[Florian Schneider, 2014-09-09 09:57:08]

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:630: ASSERT(param->base_reg() == FPREG ||
param->base_reg() == SPREG);
On 2014/09/08 17:34:06, srdjan wrote:
> Add parentheses.

Done.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.h (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.h:60: bool intrinsics_mode = false);
On 2014/09/08 17:34:06, srdjan wrote:
> s/intrinsics_mode/intrinsic_mode/

Done.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.h:328: bool intrinsic_mode_;
On 2014/09/08 17:34:06, srdjan wrote:
> const

Done.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:870: // "Deoptimization" from intrinsic
continues here.
On 2014/09/08 17:34:06, srdjan wrote:
> Maybe add comment describing why deoptimization is in quotes (deoptimization
> mechanism for redirecting to slow path).

Done.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1912: bool is_intrinsic_;
On 2014/09/08 17:34:07, srdjan wrote:
> const (also for result_)

Done.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:96: FlowGraph* graph) {
On 2014/09/08 17:34:07, srdjan wrote:
> Maybe briefly document why 'graph' is not the same as compiler->flow_graph().
> You should stress the fact that none of FlowGraphCompiler operation used
should
> rely on its own FLowGraph. Ideally we would pass something else than
> FlowGraphCompiler, but that would require major restructuring, I think.

Agree, I need it only for emitting code (assembler) and parallel move resolver.
I'll add a comment. Also this function is private to the intrinsifier, and the
builder-methods (Build_*) only take a FlowGraph as argument.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:122: FlowGraphBuilder
builder(const_cast<ParsedFunction*>(&parsed_function),
On 2014/09/08 17:34:07, srdjan wrote:
> I feel this is dangerous (removing const-ness from const ParsedFunction&
> argument) Can you restructure code so that parsed_function that is passed in
is
> not const &?

Done. That was only temporary, anyway. We have a mixure of passing pointers and
const references: FlowGraphBuilder has ParsedFunction* and
FlowGraph/FlowGraphCompiler have a const-reference. Without changing hundred
places, I'm changing FlowGraph/FlowGraphCompiler to return a non-const
reference.

https://codereview.chromium.org/513213002/diff/140001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:125: -1);  // No OSR id.
On 2014/09/08 17:34:07, srdjan wrote:
> Use Isolate::kNoDeoptId.

Done.

[srdjan, 2014-09-09 21:01:43]

lgtm

[Vyacheslav Egorov (Google), 2014-09-09 21:30:50]

lgtm

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:193:
ASSERT(input->definition()->HasSSATemp());
This assertion is redundant. There is one in BitVector::Add(...) that checks for
-1

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1790: (split_block->loop() != NULL)) {
We should have landed this change separately to isolate performance impact.

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm64.cc:1621:
compiler->RestoreLiveRegisters(locs);
Did you verify that we use a correct safe-point when GC happens here?

[Florian Schneider, 2014-09-10 13:35:29]

Committed patchset #10 (id:180001) manually as r40088 (presubmit successful).

[Florian Schneider, 2014-09-10 14:20:41]

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:193:
ASSERT(input->definition()->HasSSATemp());
On 2014/09/09 21:30:50, Vyacheslav Egorov (Google) wrote:
> This assertion is redundant. There is one in BitVector::Add(...) that checks
for
> -1

Done.

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1790: (split_block->loop() != NULL)) {
On 2014/09/09 21:30:50, Vyacheslav Egorov (Google) wrote:
> We should have landed this change separately to isolate performance impact.

I actually planned to factor it out.

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/513213002/diff/160001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm64.cc:1621:
compiler->RestoreLiveRegisters(locs);
On 2014/09/09 21:30:50, Vyacheslav Egorov (Google) wrote:
> Did you verify that we use a correct safe-point when GC happens here?

As discussed, I avoid generating the slow-path for intrinsics now and go to the
common slow-path on allocation failure.