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

Simplify constant pool usage in arm64 code generator (by removing extra argument
to all code emitting routines that may access the pool).
Improve code loading immediates on arm64 (do not read from the pool if 1 or 2
immediate instructions will do).

R=zra@google.com

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

[regis, 2015-07-29 00:35:41]

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

[Florian Schneider, 2015-07-29 09:56:05]

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

[Florian Schneider, 2015-07-29 09:56:07]

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

https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:389: LoadImmediate(dst, target);
This will have change again with precompiled code where ExternalLabel has to be
loaded from the pool. I'd rather put this change on hold until then.

[Florian Schneider, 2015-07-29 10:22:44]

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

https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:389: LoadImmediate(dst, target);
On 2015/07/29 09:56:07, Florian Schneider wrote:
> This will have change again with precompiled code where ExternalLabel has to
be
> loaded from the pool. I'd rather put this change on hold until then.

Alternatively, I would suggest to leave loading of ExternalLabels as is - i.e.
load then via the constant pool.

[regis, 2015-07-29 15:35:02]

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

https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64....
runtime/vm/assembler_arm64.cc:389: LoadImmediate(dst, target);
On 2015/07/29 10:22:44, Florian Schneider wrote:
> On 2015/07/29 09:56:07, Florian Schneider wrote:
> > This will have change again with precompiled code where ExternalLabel has to
> be
> > loaded from the pool. I'd rather put this change on hold until then.
> 
> Alternatively, I would suggest to leave loading of ExternalLabels as is - i.e.
> load then via the constant pool.

Do you mean that the external label will be patchable? So far, the patchable
argument was always kNotPatchable, which is the reason why I simplified this
code by removing the patchable formal parameter. What will be the signature of
the function? What about LoadExternalLabelFixed below? Will it change?

[regis, 2015-07-29 17:52:17]

On 2015/07/29 15:35:02, regis wrote:
>
https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64.cc
> File runtime/vm/assembler_arm64.cc (right):
> 
>
https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64....
> runtime/vm/assembler_arm64.cc:389: LoadImmediate(dst, target);
> On 2015/07/29 10:22:44, Florian Schneider wrote:
> > On 2015/07/29 09:56:07, Florian Schneider wrote:
> > > This will have change again with precompiled code where ExternalLabel has
to
> > be
> > > loaded from the pool. I'd rather put this change on hold until then.
> > 
> > Alternatively, I would suggest to leave loading of ExternalLabels as is -
i.e.
> > load then via the constant pool.
> 
> Do you mean that the external label will be patchable? So far, the patchable
> argument was always kNotPatchable, which is the reason why I simplified this
> code by removing the patchable formal parameter. What will be the signature of
> the function? What about LoadExternalLabelFixed below? Will it change?

Ryan answered my questions offline and Srdjan pointed out that you probably
meant "load *them* via the constant pool".
So I updated the cl to load the external label from the pool if using the
constant pool is allowed. It is not always allowed, e.g. when calling the
runtime from stubs. The resulting code should be identical as before, since it
is rare that the bit pattern of a label can be generated by 2 or fewer
instructions.

[zra, 2015-07-29 21:13:04]

lgtm

It would be interesting as a sanity check to see how many instructions are
generate for "Hello, world!" before and after this change.

[regis, 2015-07-29 22:20:52]

The new version executes 0.86% more arm64 instructions when running hello world
in the simulator.

This is expected, because the new code will avoid a pool access (1 instruction)
to load an immediate value that can be synthesized with 2 immediate
instructions. Note that this can easily be tuned in LoadImmediate.

Unfortunately, the simulator does not count memory reads anymore, but there
should be fewer.

Thanks,
Regis

[regis, 2015-07-29 22:21:24]

Committed patchset #2 (id:20001) manually as
b5f4e95433d0c2904815c36860eae7dff709e305 (presubmit successful).

[Florian Schneider, 2015-08-03 08:23:13]

On 2015/07/29 17:52:17, regis wrote:
> On 2015/07/29 15:35:02, regis wrote:
> >
>
https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64.cc
> > File runtime/vm/assembler_arm64.cc (right):
> > 
> >
>
https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64....
> > runtime/vm/assembler_arm64.cc:389: LoadImmediate(dst, target);
> > On 2015/07/29 10:22:44, Florian Schneider wrote:
> > > On 2015/07/29 09:56:07, Florian Schneider wrote:
> > > > This will have change again with precompiled code where ExternalLabel
has
> to
> > > be
> > > > loaded from the pool. I'd rather put this change on hold until then.
> > > 
> > > Alternatively, I would suggest to leave loading of ExternalLabels as is -
> i.e.
> > > load then via the constant pool.
> > 
> > Do you mean that the external label will be patchable? So far, the patchable
> > argument was always kNotPatchable, which is the reason why I simplified this
> > code by removing the patchable formal parameter. What will be the signature
of
> > the function? What about LoadExternalLabelFixed below? Will it change?
> 
> Ryan answered my questions offline and Srdjan pointed out that you probably
> meant "load *them* via the constant pool".
> So I updated the cl to load the external label from the pool if using the
> constant pool is allowed. It is not always allowed, e.g. when calling the
> runtime from stubs. The resulting code should be identical as before, since it
> is rare that the bit pattern of a label can be generated by 2 or fewer
> instructions.

ExternalLabels always need to be loaded from the constant pool, *also* in stubs.

The only place where the constant pool register is not set up is in intrinsic
code.

This means we can't to any runtime- or stub-calls from intrinsic code. But
intrinsics
usually fall back to the original function's code if the fast-path fails.

[regis, 2015-08-03 21:00:43]

On 2015/08/03 08:23:13, Florian Schneider wrote:
> On 2015/07/29 17:52:17, regis wrote:
> > On 2015/07/29 15:35:02, regis wrote:
> > >
> >
>
https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64.cc
> > > File runtime/vm/assembler_arm64.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1264543002/diff/1/runtime/vm/assembler_arm64....
> > > runtime/vm/assembler_arm64.cc:389: LoadImmediate(dst, target);
> > > On 2015/07/29 10:22:44, Florian Schneider wrote:
> > > > On 2015/07/29 09:56:07, Florian Schneider wrote:
> > > > > This will have change again with precompiled code where ExternalLabel
> has
> > to
> > > > be
> > > > > loaded from the pool. I'd rather put this change on hold until then.
> > > > 
> > > > Alternatively, I would suggest to leave loading of ExternalLabels as is
-
> > i.e.
> > > > load then via the constant pool.
> > > 
> > > Do you mean that the external label will be patchable? So far, the
patchable
> > > argument was always kNotPatchable, which is the reason why I simplified
this
> > > code by removing the patchable formal parameter. What will be the
signature
> of
> > > the function? What about LoadExternalLabelFixed below? Will it change?
> > 
> > Ryan answered my questions offline and Srdjan pointed out that you probably
> > meant "load *them* via the constant pool".
> > So I updated the cl to load the external label from the pool if using the
> > constant pool is allowed. It is not always allowed, e.g. when calling the
> > runtime from stubs. The resulting code should be identical as before, since
it
> > is rare that the bit pattern of a label can be generated by 2 or fewer
> > instructions.
> 
> ExternalLabels always need to be loaded from the constant pool, *also* in
stubs.
> 
> The only place where the constant pool register is not set up is in intrinsic
> code.
> 
> This means we can't to any runtime- or stub-calls from intrinsic code. But
> intrinsics
> usually fall back to the original function's code if the fast-path fails.

Our current code is not following that guideline. Maybe we should fix it.
An example is the UpdateStoreBuffer stub (e.g. stub_code_arm64.cc:1075), which
calls into the runtime without a pool pointer setup.
Should that stub set it up? Or should EnterCallRuntimeFrame set one up?

If you'd like, I can go ahead and fix all occurrences I find. Let me know.

Thanks