Simplify constant pool usage in x64 code generator (by removing extra argument

Simplify constant pool usage in x64 code generator (by removing extra argument
to all code emitting routines that may access the pool).

R=zra@google.com

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

[regis, 2015-07-30 22:36:09]

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

[srdjan, 2015-07-30 22:44:11]

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

[srdjan, 2015-07-30 22:44:12]

dbc

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_x64.cc (left):

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:77:
assembler()->set_constant_pool_allowed(true);
SHouldn't this be set (if at all) to whatever it was before and not always tot
to true?

[regis, 2015-07-30 23:04:16]

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_x64.cc (left):

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:77:
assembler()->set_constant_pool_allowed(true);
On 2015/07/30 22:44:11, srdjan wrote:
> SHouldn't this be set (if at all) to whatever it was before and not always tot
> to true?

The improved code maintains the state of constant_pool_allowed_ according to the
action it performs on the pool pointer, e.g. set it to true when loading a pool
pointer, or set it to false when restoring a pool pointer it does not own.

It also checks with an assert that the current state is what it expects. But it
does set the state out of the blue. There is no action performed here.

There is no state to check either here, because a particular intrinsic is free
to setup and use a pool pointer.

[zra, 2015-07-31 17:37:28]

lgtm

Now that the change is being made to x64, we can try to keep an eye on
performance.

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:1503: LoadImmediate(TMP, imm);
assert reg != TMP

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:1513: LoadImmediate(TMP, imm);
Could address refer to TMP?

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:2818: void
Assembler::LoadFunctionFromNewPool(Register dst,
Maybe FromCalleePool instead of FromNewPool.

https://codereview.chromium.org/1268783003/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/1268783003/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:109: __ set_constant_pool_allowed(true);
I guess it's not possible for ReturnInstr to be emitted someplace where the
constant pool is not allowed, but its  still slightly scary to see this
hardcoded to true at this abstraction level. Maybe add an assert before
LeaveDartFrame?

[regis, 2015-07-31 19:33:59]

Thanks!

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:1503: LoadImmediate(TMP, imm);
On 2015/07/31 17:37:28, zra wrote:
> assert reg != TMP

Done here and at a bunch of other places.

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:1513: LoadImmediate(TMP, imm);
On 2015/07/31 17:37:28, zra wrote:
> Could address refer to TMP?

Since I did not change this, we would know by now if it was the case.

https://codereview.chromium.org/1268783003/diff/1/runtime/vm/assembler_x64.cc...
runtime/vm/assembler_x64.cc:2818: void
Assembler::LoadFunctionFromNewPool(Register dst,
On 2015/07/31 17:37:28, zra wrote:
> Maybe FromCalleePool instead of FromNewPool.

Done.

https://codereview.chromium.org/1268783003/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/1268783003/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:109: __ set_constant_pool_allowed(true);
On 2015/07/31 17:37:28, zra wrote:
> I guess it's not possible for ReturnInstr to be emitted someplace where the
> constant pool is not allowed, but its  still slightly scary to see this
> hardcoded to true at this abstraction level. Maybe add an assert before
> LeaveDartFrame?

Done.

[regis, 2015-07-31 19:38:38]

Committed patchset #3 (id:40001) manually as
cd00cf3f8e6e96a95dad9c6ebcfbb477ee7bc3f2 (presubmit successful).