Move CreateArray and CreateClosure to new location template.

Move CreateArray and CreateClosure to new location template.
Committed: https://code.google.com/p/dart/source/detail?r=8104

[Vyacheslav Egorov (Google), 2012-05-30 10:44:11]

https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:600: compiler->LoadValue(RBX,
element_type());
New templates should not use LoadValue directly. Instead RBX should become an
input.

I can see however that our instruction prologue does not have a proper support
for constant inputs though. So it will try to pop something that is not pushed
if you try that.

It seems that the only way around is to teach prologue about connection between
inputs locations and input values, so that it could support both constants and
non-constants.

[Vyacheslav Egorov (Google), 2012-05-30 11:13:41]

Sorry for the confusion. No need to implement constants support, I misread
BuildInstantiatedTypeArguments

https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language_x64.cc:600: compiler->LoadValue(RBX,
element_type());
On 2012/05/30 10:44:11, Vyacheslav Egorov (Google) wrote:
> New templates should not use LoadValue directly. Instead RBX should become an
> input.
> 
> I can see however that our instruction prologue does not have a proper support
> for constant inputs though. So it will try to pop something that is not pushed
> if you try that.
> 
> It seems that the only way around is to teach prologue about connection
between
> inputs locations and input values, so that it could support both constants and
> non-constants.

Actually I misread the code in BuildInstantiatedTypeArguments. It will never
return a constant directly even if it is null. So you can just convert Remove
load value and do

locs->set_in(0, Location::RegisterLocation(RBX))

[regis, 2012-05-30 11:24:04]

On 2012/05/30 11:13:41, Vyacheslav Egorov (Google) wrote:
> Sorry for the confusion. No need to implement constants support, I misread
> BuildInstantiatedTypeArguments
> 
>
https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
> File runtime/vm/intermediate_language_x64.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
> runtime/vm/intermediate_language_x64.cc:600: compiler->LoadValue(RBX,
> element_type());
> On 2012/05/30 10:44:11, Vyacheslav Egorov (Google) wrote:
> > New templates should not use LoadValue directly. Instead RBX should become
an
> > input.
> > 
> > I can see however that our instruction prologue does not have a proper
support
> > for constant inputs though. So it will try to pop something that is not
pushed
> > if you try that.
> > 
> > It seems that the only way around is to teach prologue about connection
> between
> > inputs locations and input values, so that it could support both constants
and
> > non-constants.
> 
> Actually I misread the code in BuildInstantiatedTypeArguments. It will never
> return a constant directly even if it is null. So you can just convert Remove
> load value and do
> 
> locs->set_in(0, Location::RegisterLocation(RBX))

I had come to the same conclusion. Please, have another look.

Thanks,
Regis

[regis, 2012-05-30 11:27:48]

On 2012/05/30 11:24:04, regis wrote:
> On 2012/05/30 11:13:41, Vyacheslav Egorov (Google) wrote:
> > Sorry for the confusion. No need to implement constants support, I misread
> > BuildInstantiatedTypeArguments
> > 
> >
>
https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
> > File runtime/vm/intermediate_language_x64.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10460002/diff/1/runtime/vm/intermediat...
> > runtime/vm/intermediate_language_x64.cc:600: compiler->LoadValue(RBX,
> > element_type());
> > On 2012/05/30 10:44:11, Vyacheslav Egorov (Google) wrote:
> > > New templates should not use LoadValue directly. Instead RBX should become
> an
> > > input.
> > > 
> > > I can see however that our instruction prologue does not have a proper
> support
> > > for constant inputs though. So it will try to pop something that is not
> pushed
> > > if you try that.
> > > 
> > > It seems that the only way around is to teach prologue about connection
> > between
> > > inputs locations and input values, so that it could support both constants
> and
> > > non-constants.
> > 
> > Actually I misread the code in BuildInstantiatedTypeArguments. It will never
> > return a constant directly even if it is null. So you can just convert
Remove
> > load value and do
> > 
> > locs->set_in(0, Location::RegisterLocation(RBX))
> 
> I had come to the same conclusion. Please, have another look.
> 
> Thanks,
> Regis

One more thing. The elements of the array are implicitly passed on the stack and
are not considered as inputs. I am not sure this is kosher with the new location
template.

Thanks,
Regis

[Vyacheslav Egorov (Google), 2012-05-30 11:42:17]

lgtm

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:585: const intptr_t kNumTemps = 2;
should be 1

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:614: }
On the one hand: it has all arguments on the stack and behaves like a call (and
does a call inside). So we could just MakeCallSummary() and load RBX explicitly
from the stack in the template.

On the other hand: we would not want to load something from the stack (like a
null constant) if we can have it loaded directly into register --- so loading
EBX is better left outside of the template.

I think what we have now is perfectly fine. But I defer it for you to decide. 

What I would do is to add TODO into MakeLocationSummary to keep in mind that
this instruction is a call (because real register allocator would have to know
that) and leave everything as it is in this version.

[Florian Schneider, 2012-05-30 12:38:12]

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
runtime/vm/intermediate_language.h:1026: virtual intptr_t InputCount() const;
In the existing code I think this function reports the wrong number of
inputs/arguments: There is an optional receiver pushed if
IsImplicitInstanceClosure is true. So the count can be 0, 1 or 2.

It works right now because the generated code drops the correct number of
arguments and we don't use the stack-temp index for anything yet, but will be a
problem in the future.

[regis, 2012-05-30 13:41:09]

Thanks!

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
runtime/vm/intermediate_language.h:1026: virtual intptr_t InputCount() const;
On 2012/05/30 12:38:12, Florian Schneider wrote:
> In the existing code I think this function reports the wrong number of
> inputs/arguments: There is an optional receiver pushed if
> IsImplicitInstanceClosure is true. So the count can be 0, 1 or 2.
> 
> It works right now because the generated code drops the correct number of
> arguments and we don't use the stack-temp index for anything yet, but will be
a
> problem in the future.

Done.

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:585: const intptr_t kNumTemps = 2;
On 2012/05/30 11:42:17, Vyacheslav Egorov (Google) wrote:
> should be 1

Done.

https://chromiumcodereview.appspot.com/10460002/diff/3002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:614: }
On 2012/05/30 11:42:17, Vyacheslav Egorov (Google) wrote:
> On the one hand: it has all arguments on the stack and behaves like a call
(and
> does a call inside). So we could just MakeCallSummary() and load RBX
explicitly
> from the stack in the template.
> 
> On the other hand: we would not want to load something from the stack (like a
> null constant) if we can have it loaded directly into register --- so loading
> EBX is better left outside of the template.
> 
> I think what we have now is perfectly fine. But I defer it for you to decide. 
> 
> What I would do is to add TODO into MakeLocationSummary to keep in mind that
> this instruction is a call (because real register allocator would have to know
> that) and leave everything as it is in this version. 

Added comment.

[srdjan, 2012-05-30 15:58:37]

LGTM with comments

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:603: __ movq(R10,
Immediate(Smi::RawValue(ElementCount())));
Use temp instead of R10, so that the knowledge of R10 is embedded onlhy in
MakeLocationSumary.

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:610: __ leaq(R10, FieldAddress(RAX,
Array::data_offset()));
ditto here and below.

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:615: compiler->LoadValue(RBX,
ElementAt(i));
I think this should no happend, i.e., FlowGraphBuilder should make sure that we
have only uses as inputs.

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:637: __ Drop(1);  // Discard type
arguments.
Could we normalize the closure creation to always requite a type argument
(either Null or nothing)?

[regis, 2012-05-30 16:59:08]

Thanks. All done in another cl (issue 10448079).

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:603: __ movq(R10,
Immediate(Smi::RawValue(ElementCount())));
On 2012/05/30 15:58:37, srdjan wrote:
> Use temp instead of R10, so that the knowledge of R10 is embedded onlhy in
> MakeLocationSumary.

Done.

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:610: __ leaq(R10, FieldAddress(RAX,
Array::data_offset()));
On 2012/05/30 15:58:37, srdjan wrote:
> ditto here and below.

Done.

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:615: compiler->LoadValue(RBX,
ElementAt(i));
On 2012/05/30 15:58:37, srdjan wrote:
> I think this should no happend, i.e., FlowGraphBuilder should make sure that
we
> have only uses as inputs.

Done.

https://chromiumcodereview.appspot.com/10460002/diff/4002/runtime/vm/intermed...
runtime/vm/intermediate_language_x64.cc:637: __ Drop(1);  // Discard type
arguments.
On 2012/05/30 15:58:37, srdjan wrote:
> Could we normalize the closure creation to always requite a type argument
> (either Null or nothing)?

Done.