1438800 - MacroAssembler::splitTagForTest() returns some random scratch register, brittle API, breaks ARM64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Lars T Hansen [:lth]]

splitTagForTest() takes a ValueOperand and returns a Register.  Until now this register has been some platform-dependent scratch register, ideally one that won't be needed for operating on that register subsequently, eg for equality testing.

This *just* works because x86 has large immediates and ARM and MIPS return the *other* scratch register, the one the assembler is not likely to use very soon...  

(You see where this is going.)

On ARM64 this happens not to work because the scratch registers are heavily used by the assembler and are allocated differently.  Basically this is not a workable system and we just need to clean it up, the sooner the better.  The caller should pass in a suitable register to hold the tag, and that register should be held in a scoped manner by the caller, eg, with ScratchRegisterScope or similar.

Comment 1

[Lars T Hansen [:lth]]

The truth is slightly more complicated still: ARM and x86-32 and MIPS32 return the typeReg of the ValueOperand, so don't need a scratch register; x64 returns ScratchReg, MIPS64 returns SecondScratchReg, ARM64 would presumably erect a scratch register scope and take one of the scratch registers.

Looks like this functionality can be packaged up fairly nicely.  A few assertions currently because the scratch register is reused but that was expected; it can be worked around.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1438800-scratch-tag-scope.patch

Tentative fix, needs more testing (but passes all jit tests so far).  This introduces RAII types to manage a register for splitTagForTest(), so there should be no regression from this, just the turbulence from handling registers properly.

Comment 3

[Lars T Hansen [:lth]]

First try run.  Christmasy, because we need to free up the now scope-allocated register a little more often.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b30001e8433fa2d9d7b19625240badb482648e

Comment 4

[Lars T Hansen [:lth]]

With this patch (and some adjustments), most ARM64 tests pass with --baseline-eager.  (Not all stubs work is complete but at least now the EnterJit stub is looking OK.)

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1438800-scratch-tag-scope-v2.patch

This introduces two RAII types to manage the scratch register for tag testing, one to bind it and one to release it in a dynamic scope.  See commit msg and comment block in MacroAssembler-x64.h for more.

This change interacts with ScratchRegisterScope / SecondScratchRegisterScope on several platforms, and as a side effect, this change clarifies the regions where the tag register goes out of scope.  As a result the diff is somewhat large but the clarity is welcome, I expect.

This is not the only way to fix the problem, but it is friendly to the temp register management we already do on ARM64, which is different from the ScratchRegisterScope system we use elsewhere (sadly).  The alternative to this solution would be to revamp all of the ARM64 back-end to use ScratchRegisterScope, but that's a huge and disruptive job.

Third try run is finally green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc5ffe1a0d4cadad2b6638ec00e0da761e48dedf

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8952075 [details] [diff] [review]
bug1438800-scratch-tag-scope-v2.patch

Review of attachment 8952075 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense. I think eventually we should remove splitTagForTest completely and just pass an explicit scratch register; maybe file a follow-up bug.

::: js/src/jit/CodeGenerator.cpp
@@ +1121,5 @@
>      const JSAtomState& names = gen->runtime->names();
>  
> +    {
> +        ScratchTagScope tag(masm, input);
> +        masm.splitTagForTest(input, tag);

I think here it's simpler to do:

  Register tag = masm.extractTag(input, output);

tag will be input.typeReg() on 32-bit, but on 64-bit this will use the output register instead of a random scratch register.

@@ +10912,5 @@
>  
>      MOZ_ASSERT_IF(!input->emptyResultTypeSet(), numTests > 0);
>  
> +    ScratchTagScope tag(masm, value);
> +    masm.splitTagForTest(value, tag);

Same here.

@@ +11104,5 @@
>                                     StoreValueTo(out));
>  
> +    {
> +        ScratchTagScope tag(masm, input);
> +        masm.splitTagForTest(input, tag);

Same here with out.scratchReg().

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1438800-scratch-tag-scope-v3.patch

Rebased, ready to land.  Carrying r+.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4a3fb51e9e
Introduce ScratchTagScope.  r=jandem

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/9f4a3fb51e9e