1215999 - ARM64: Remove unnecessary float registers definitions.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Victor Carlquist]

As Nicolas commented on bug 1211150 comment 10, the ReturnFloatReg and ScratchFloatReg seems to be aliases of ScratchDoubleReg and ReturnDoubleReg, 
so we could remove them or, maybe, set them with ScratchDoubleReg and ReturnDoubleReg respectively.

We should do the same with the FloatReg0 and FloatReg1.

Comment 1

[Victor Carlquist]

Attachment: Patch

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8676466 [details] [diff] [review]
Patch

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

Sorry, I don't follow what is happening here.

It's true that FloatReturnReg and DoubleReturnReg are aliasing each other, but they don't have the same size. One is 32 bits and the other is 64 bits.

Similarly, v0 is a 128-bit register whose low 64 bits alias d0.

Comment 3

[Sean Stangl [:sstangl]]

The FloatReg0 definition is erroneous and causes breakage on ARM64 tests, since it is defined as s0 but used as d0. In this case, "Float" means "Double", and "Float32" means "Single". They are named pretty badly.

FloatReg1 cannot be ScratchFloatReg, though, since that may be independently acquired within the MacroAssembler at any point.

In the context of this patch, v0 is an integer register code such that s0 == d0 == v0.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Sean Stangl [:sstangl] from comment #3)
> The FloatReg0 definition is erroneous and causes breakage on ARM64 tests,
> since it is defined as s0 but used as d0. In this case, "Float" means
> "Double", and "Float32" means "Single". They are named pretty badly.
> 
> FloatReg1 cannot be ScratchFloatReg, though, since that may be independently
> acquired within the MacroAssembler at any point.
> 
> In the context of this patch, v0 is an integer register code such that s0 ==
> d0 == v0.

Thanks, Sean.

I can't find any uses of ReturnFloatReg with grep. Is there some kind of macro trickery in play, or can we delete it?

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Same for ScratchFloatReg: It's unused.

Comment 6

[Victor Carlquist]

Okay, thanks, I'll fix it.

Comment 7

[Jakob Stoklund Olesen [:jolesen]]

Thanks, Victor.

I assume that the bad FloatReg0 definition is the cause of these assertions: 
    dist/bin/js ../js/src/jit-test/tests/basic/mod.js
    Assertion failure: !move.from().aliases(move.to()), at /Users/jolesen/gecko-dev/js/src/jit/MoveResolver.cpp:269
    Segmentation fault: 11

Comment 8

[Victor Carlquist]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #7)
> Thanks, Victor.
> 
> I assume that the bad FloatReg0 definition is the cause of these assertions: 
>     dist/bin/js ../js/src/jit-test/tests/basic/mod.js
>     Assertion failure: !move.from().aliases(move.to()), at
> /Users/jolesen/gecko-dev/js/src/jit/MoveResolver.cpp:269
>     Segmentation fault: 11

Yep!

Comment 9

[Victor Carlquist]

Attachment: Patch

Comment 10

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8676552 [details] [diff] [review]
Patch

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

Looks good. Thanks!

::: js/src/jit/arm64/SharedICRegisters-arm64.h
@@ +49,5 @@
>  // register.  In ARM code emission, we do not clobber BaselineTailCallReg
>  // since we keep the return address for calls there.
>  
> +static constexpr FloatRegister FloatReg0 = { FloatRegisters::v0, FloatRegisters::Double };
> +static constexpr FloatRegister FloatReg1 = { FloatRegisters::v1, FloatRegisters::Double };

Since these registers are used as 64-bit doubles, I would prefer to use 'd0' and 'd1' instead of 'v0' and 'v1'. They have the same numerical value.

Comment 11

[Victor Carlquist]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #10)
> Since these registers are used as 64-bit doubles, I would prefer to use 'd0'
> and 'd1' instead of 'v0' and 'v1'. They have the same numerical value.

Ok, thanks for your review.

Comment 12

[Victor Carlquist]

Attachment: Patch

Comment 13

[Victor Carlquist]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22e971f69c5b

Comment 14

[Victor Carlquist]

Try is green, so I am setting the "checkin-needed" keyword.

Thanks!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66dae447c298

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66dae447c298