Cache current thread in a reserved register and use it in LoadIsolate

Cache current thread in a reserved register and use it in LoadIsolate

Add THR register that caches the current thread in generated code.
Shuffle some registers around as needed to free one up.

Note: Assembler::LoadIsolate now always loads the *current* isolate whenever the code is executed, rather than the isolate in which the code was compiled (no existing code was affected by this slight change in semantics).

Rewrite some ia32 stubs to use one less register, and for some ia32 bigint intrinsics, explicitly save THR (like CTX in the past, see r44699).

Next steps:
- pass current thread rather than isolate in NativeArguments
- ditto for exception handler jump
- migrate fields vm_tag, top_exit_frame_info, etc. to thread

R=srdjan@google.com

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

[koda, 2015-05-22 02:31:52]

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

[srdjan, 2015-05-26 20:10:44]

lgtm

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

https://codereview.chromium.org/1156593002/diff/40001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:27: // The FP register should not be modified,
because it is used by the profiler.
Mention THR(R8) as not allowed to be modified,

https://codereview.chromium.org/1156593002/diff/40001/runtime/vm/stub_code_ia...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/1156593002/diff/40001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:51: __ LoadIsolate(EDI);
This destroys the context register, I assume it does not matter at this point.

[koda, 2015-05-26 20:29:12]

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

https://codereview.chromium.org/1156593002/diff/40001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:27: // The FP register should not be modified,
because it is used by the profiler.
On 2015/05/26 20:10:44, srdjan wrote:
> Mention THR(R8) as not allowed to be modified,

Done.

https://codereview.chromium.org/1156593002/diff/40001/runtime/vm/stub_code_ia...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/1156593002/diff/40001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:51: __ LoadIsolate(EDI);
On 2015/05/26 20:10:44, srdjan wrote:
> This destroys the context register, I assume it does not matter at this point.

Yes, CTX is only assumed/guaranteed to contain the context when first entering a
function.

[koda, 2015-05-26 20:46:18]

Committed patchset #4 (id:60001) manually as
fd890f4097a8c52652e0e3bf3ba22f77d69904f0 (presubmit successful).

[Florian Schneider, 2015-05-27 08:09:09]

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

https://codereview.chromium.org/1156593002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1514: | MaskBit(PP);
You need to add THR to the blocked_mask here as well. Otherwise the parallel
move resolver may use it as scratch register.

[koda, 2015-05-27 13:41:43]

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

https://codereview.chromium.org/1156593002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1514: | MaskBit(PP);
On 2015/05/27 08:09:09, Florian Schneider wrote:
> You need to add THR to the blocked_mask here as well. Otherwise the parallel
> move resolver may use it as scratch register.

Thanks for catching this. That all tests still passed in debug mode makes me
worried about our test coverage. Any suggestions for how to write a good
regression test?

For the record, this is the fix:
https://codereview.chromium.org/1158943002/

[Florian Schneider, 2015-05-27 13:57:36]

On 2015/05/27 13:41:43, koda wrote:
>
https://codereview.chromium.org/1156593002/diff/60001/runtime/vm/flow_graph_c...
> File runtime/vm/flow_graph_compiler.cc (right):
> 
>
https://codereview.chromium.org/1156593002/diff/60001/runtime/vm/flow_graph_c...
> runtime/vm/flow_graph_compiler.cc:1514: | MaskBit(PP);
> On 2015/05/27 08:09:09, Florian Schneider wrote:
> > You need to add THR to the blocked_mask here as well. Otherwise the parallel
> > move resolver may use it as scratch register.
> 
> Thanks for catching this. That all tests still passed in debug mode makes me
> worried about our test coverage. Any suggestions for how to write a good
> regression test?
> 

Not really - a Dart program that would reliablymake the parallel move resolver
to use THR as scratch register. Something with loops and a lot of shuffling of
local variables probably would work... but I don't have a concrete example.

[srdjan, 2015-05-27 15:09:39]

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

https://codereview.chromium.org/1156593002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1514: | MaskBit(PP);
On 2015/05/27 13:41:42, koda wrote:
> On 2015/05/27 08:09:09, Florian Schneider wrote:
> > You need to add THR to the blocked_mask here as well. Otherwise the parallel
> > move resolver may use it as scratch register.
> 
> Thanks for catching this. That all tests still passed in debug mode makes me
> worried about our test coverage. Any suggestions for how to write a good
> regression test?
> 
> For the record, this is the fix:
> https://codereview.chromium.org/1158943002/

You may want to run all tests with --optimization-counyter-threshold=10 (we do
it on bots) which would stress the register allocation more, and maybe catch the
issue.