ARM64: Avoid regeneration of constants.

ARM64: Avoid regeneration of constants.

At the lithium level, HConstants are cached and their virtual register reused to
avoid regenerating them.

[Alexandre Rames, 2014-06-02 09:16:05]

This shows some improvement (up to ~5%) for some of the Octane benchmarks (up to
~5% for Raytrace and Richards). The impact depends on the CPU benchmarked.

Ideally the same result should be achieved by modifying HConstant::EmitAtUses(),
but an attempt to do so triggered crashes.
For example trying:

--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -2870,6 +2870,7 @@ bool HConstant::EmitAtUses() {
   if (IsCell()) return false;
   if (representation().IsDouble()) return false;
   if (representation().IsExternal()) return false;
+  if (representation().IsTagged()) return false;
   return true;
 }

yields

  # Fatal error in ../src/arm64/lithium-codegen-arm64.cc, line 1856
  # CHECK(!info()->IsStub()) failed

Any pointer or ideas on what may be going wrong?

[danno, 2014-06-02 09:28:25]

DBC: Changes EmitAtUses to allocate Tagged constants in registers is generally a
very bad thing for i32, so you will need more platform-specific machinery to
abstract the decision.

Could you provide more context for the ASSERT, perhaps the first few frames of
the stack crawl?

[Alexandre Rames, 2014-06-02 09:37:54]

On 2014/06/02 09:28:25, danno wrote:
> DBC: Changes EmitAtUses to allocate Tagged constants in registers is generally
a
> very bad thing for i32, so you will need more platform-specific machinery to
> abstract the decision.

Of course! This is just to highlight the bug met.

> 
> Could you provide more context for the ASSERT, perhaps the first few frames of
> the stack crawl?

Below is a backtrace. Is this something 'expected' (we know we need to emit at
uses for tagged constants and some others)? Or would we expect to be able to
modify EmitAtUses() and everything to just work (let's be optimistic!)?

$ make arm64.debug -j45 snapshot=off
[...]
$ gdb --args ./out/arm64.debug/d8
(gdb) run
#
# Fatal error in ../src/arm64/lithium-codegen-arm64.cc, line 1856
# CHECK(!info()->IsStub()) failed
#
[...]
Program received signal SIGILL, Illegal instruction.
v8::internal::OS::Abort () at ../src/platform-posix.cc:253
253	    V8_IMMEDIATE_CRASH();
(gdb) bt
#0  v8::internal::OS::Abort () at ../src/platform-posix.cc:253
#1  0x00000000008c3b28 in V8_Fatal (file=0x1088560
"../src/arm64/lithium-codegen-arm64.cc", line=1856, format=0x10869a8 "CHECK(%s)
failed") at ../src/checks.cc:88
#2  0x0000000000d0f6f8 in v8::internal::LCodeGen::DoBranch (this=0x7fffffff2a30,
instr=0x1ffcd28) at ../src/arm64/lithium-codegen-arm64.cc:1856
#3  0x0000000000cea091 in v8::internal::LBranch::CompileToNative
(this=0x1ffcd28, generator=0x7fffffff2a30) at ../src/arm64/lithium-arm64.cc:20
#4  0x0000000000da3e90 in v8::internal::LCodeGenBase::GenerateBody
(this=0x7fffffff2a30) at ../src/lithium-codegen.cc:95
#5  0x0000000000d09377 in v8::internal::LCodeGen::GenerateCode
(this=0x7fffffff2a30) at ../src/arm64/lithium-codegen-arm64.cc:601
#6  0x0000000000ab5fe4 in v8::internal::LChunk::Codegen (this=0x1ffc788) at
../src/lithium.cc:441
#7  0x00000000008d9808 in
v8::internal::DoGenerateCode<v8::internal::CompareNilICStub>
(stub=0x7fffffffd0e0) at ../src/code-stubs-hydrogen.cc:274
#8  0x00000000008d2b9e in v8::internal::CompareNilICStub::GenerateCode
(this=0x7fffffffd0e0) at ../src/code-stubs-hydrogen.cc:849
#9  0x00000000008c5ec2 in v8::internal::CodeStub::GetCode (this=0x7fffffffd0e0)
at ../src/code-stubs.cc:122
#10 0x00000000008c5af4 in v8::internal::CodeStub::GetCodeCopy
(this=0x7fffffffd0e0, pattern=...) at ../src/code-stubs.cc:69
#11 0x0000000000c5398d in v8::internal::StubCache::ComputeCompareNil
(this=0x1ebef40, receiver_map=..., stub=...) at ../src/stub-cache.cc:315
#12 0x0000000000a7205f in v8::internal::CompareNilIC::CompareNil
(this=0x7fffffffd180, object=...) at ../src/ic.cc:2963
#13 0x0000000000a721da in v8::internal::__RT_impl_CompareNilIC_Miss (args=...,
isolate=0x1ea5130) at ../src/ic.cc:2976
#14 0x0000000000a7214c in v8::internal::CompareNilIC_Miss (args_length=1,
args_object=0x7ffff6dcd010, isolate=0x1ea5130) at ../src/ic.cc:2972
#15 0x0000000000d53621 in v8::internal::Simulator::DoRuntimeCall
(this=0x1ed02d0, instr=0x1eecc38) at ../src/arm64/simulator-arm64.cc:589
#16 0x0000000000d5db4b in v8::internal::Simulator::VisitException
(this=0x1ed02d0, instr=0x1eecc38) at ../src/arm64/simulator-arm64.cc:3584
#17 0x0000000000d64d26 in
v8::internal::Decoder<v8::internal::Simulator>::DecodeBranchSystemException
(this=0x1ed02d0, instr=0x1eecc38) at ../src/arm64/decoder-arm64-inl.h:156
#18 0x0000000000d63f3b in v8::internal::Decoder<v8::internal::Simulator>::Decode
(this=0x1ed02d0, instr=0x1eecc38) at ../src/arm64/decoder-arm64-inl.h:62
#19 0x0000000000d51204 in v8::internal::Simulator::ExecuteInstruction
(this=0x1ed02d0) at ../src/arm64/simulator-arm64.h:320
#20 0x0000000000d53081 in v8::internal::Simulator::Run (this=0x1ed02d0) at
../src/arm64/simulator-arm64.cc:436
#21 0x0000000000d52584 in v8::internal::Simulator::CheckPCSComplianceAndRun
(this=0x1ed02d0) at ../src/arm64/simulator-arm64.cc:248
#22 0x0000000000d52101 in v8::internal::Simulator::CallVoid (this=0x1ed02d0,
entry=0x5461eec0 "?", args=0x7fffffffd670) at
../src/arm64/simulator-arm64.cc:162
#23 0x0000000000d5214d in v8::internal::Simulator::CallInt64 (this=0x1ed02d0,
entry=0x5461eec0 "?", args=0x7fffffffd670) at
../src/arm64/simulator-arm64.cc:169
#24 0x0000000000d52275 in v8::internal::Simulator::CallJS (this=0x1ed02d0,
entry=0x5461eec0 "?", function_entry=0x54666c60 "\234\203", func=0x32f51559,
revc=0x32f10c21, argc=0, 
    argv=0x0) at ../src/arm64/simulator-arm64.cc:194
#25 0x000000000094ff67 in v8::internal::Invoke (is_construct=false,
function=..., receiver=..., argc=0, args=0x0) at ../src/execution.cc:94
#26 0x0000000000950319 in v8::internal::Execution::Call (isolate=0x1ea5130,
callable=..., receiver=..., argc=0, argv=0x0, convert_receiver=false) at
../src/execution.cc:149
#27 0x00000000008abd39 in v8::internal::Genesis::CompileScriptCached
(isolate=0x1ea5130, name=..., source=..., cache=0x0, extension=0x0,
top_context=..., use_runtime_context=true)
    at ../src/bootstrapper.cc:1551
#28 0x00000000008ab9bf in v8::internal::Genesis::CompileNative
(isolate=0x1ea5130, name=..., source=...) at ../src/bootstrapper.cc:1493
#29 0x00000000008ab804 in v8::internal::Genesis::CompileBuiltin
(isolate=0x1ea5130, index=19) at ../src/bootstrapper.cc:1459
#30 0x00000000008af551 in v8::internal::Genesis::InstallNatives
(this=0x7fffffffde20) at ../src/bootstrapper.cc:1881
#31 0x00000000008b2c3b in v8::internal::Genesis::Genesis (this=0x7fffffffde20,
isolate=0x1ea5130, global_object=..., global_template=...,
extensions=0x7fffffffdf90)
    at ../src/bootstrapper.cc:2601
#32 0x00000000008a5e4f in v8::internal::Bootstrapper::CreateEnvironment
(this=0x1ebee90, global_object=..., global_template=...,
extensions=0x7fffffffdf90)
    at ../src/bootstrapper.cc:326
#33 0x000000000087ebb5 in v8::CreateEnvironment (isolate=0x1ea5130,
extensions=0x7fffffffdf90, global_template=..., global_object=...) at
../src/api.cc:5124
#34 0x000000000087ed87 in v8::Context::New (external_isolate=0x1ea5130,
extensions=0x7fffffffdf90, global_template=..., global_object=...) at
../src/api.cc:5154
#35 0x000000000084d107 in v8::Shell::InitializeDebugger (isolate=0x1ea5130) at
../src/d8.cc:906
#36 0x00000000008548ab in v8::Shell::Main (argc=1, argv=0x7fffffffe248) at
../src/d8.cc:1509
#37 0x0000000000854b80 in main (argc=1, argv=0x7fffffffe248) at
../src/d8.cc:1568

[ulan, 2014-06-03 07:33:36]

I am not sure why !info()->IsStub() check is triggering, but the approach with
EmitAtUses seems much better. As Danno mentioned we would need a predicate for
each architecture that decides if it is worthwhile to store constants in
registers.

[danno, 2014-06-03 08:01:16]

Actually, the right thing to do is to treat constants as normal values and teach
the register allocator how to split constant ranges and rematerialize on demand
without spilling. This is something that Jaro and Benedikt have thought about
recently, and I think this is generally a cleaner way to approach the problem
than the ad-hoc "EmitAsUses" approach which is just a brittle heuristic.

[Alexandre Rames, 2014-06-03 08:27:25]

On 2014/06/03 08:01:16, danno wrote:
> Actually, the right thing to do is to treat constants as normal values and
teach
> the register allocator how to split constant ranges and rematerialize on
demand
> without spilling. This is something that Jaro and Benedikt have thought about
> recently, and I think this is generally a cleaner way to approach the problem
> than the ad-hoc "EmitAsUses" approach which is just a brittle heuristic.

I did a bit of work on this as a follow-up for this patch:
I augmented lithium instructions with a method indicating if it was better to
regenerate the value (re-visit the instruction) instead of spilling and
restoring it. LiveRanges and LOperands kept track of which lithium instruction
output they referred to. The gap resolver skips spilling moves when appropriate,
and instead of moving from the stack re-visits the lithium instruction.
I think there is some more work to do around that, but if you are interested I
could upload this so you can have a look.

[Jarin, 2014-06-03 11:04:35]

On 2014/06/03 08:27:25, Alexandre Rames wrote:
> On 2014/06/03 08:01:16, danno wrote:
> > Actually, the right thing to do is to treat constants as normal values and
> teach
> > the register allocator how to split constant ranges and rematerialize on
> demand
> > without spilling. This is something that Jaro and Benedikt have thought
about
> > recently, and I think this is generally a cleaner way to approach the
problem
> > than the ad-hoc "EmitAsUses" approach which is just a brittle heuristic.
> 
> I did a bit of work on this as a follow-up for this patch:
> I augmented lithium instructions with a method indicating if it was better to
> regenerate the value (re-visit the instruction) instead of spilling and
> restoring it. LiveRanges and LOperands kept track of which lithium instruction
> output they referred to. The gap resolver skips spilling moves when
appropriate,
> and instead of moving from the stack re-visits the lithium instruction.
> I think there is some more work to do around that, but if you are interested I
> could upload this so you can have a look.

Yes, it would be great if you could share your work. (I am currently working on
a different optimisation in the register allocator, but I was thinking about
doing the rematerialization, too.)

[Alexandre Rames, 2014-06-12 13:08:08]

Any progress on this topic?

[Jarin, 2014-06-12 15:12:06]

On 2014/06/12 13:08:08, Alexandre Rames wrote:
> Any progress on this topic?

There seem to be too many complications in the register allocator to fix the
constant handling there, so we decided against it (for now).

One of the reasons why EmitAtUses() == false does not work is lowering of
HParameters in registers (used by hydrogen stubs): Lowering of register
HParameter relies on the register not being clobbered until we reach the
parameter instruction; however, eager emission of constants is quite happy to
put the constants into registers and thus overwrite the parameters. All this is
fixable (e.g. by adding live ranges for parameters from the beginning of the
function), but it seems to be a lot of work.

I am sorry.