Closed Bug 1215999 Opened 4 years ago Closed 4 years ago

ARM64: Remove unnecessary float registers definitions.

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: victorcarlquist, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8676466 - Flags: review?(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.
Attachment #8676466 - Flags: review?(jolesen)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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?
Same for ScratchFloatReg: It's unused.
Okay, thanks, I'll fix it.
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
(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!
Attached patch Patch (obsolete) — Splinter Review
Attachment #8676552 - Flags: review?(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.
Attachment #8676552 - Flags: review?(jolesen) → review+
(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.
Attached patch PatchSplinter Review
Attachment #8676466 - Attachment is obsolete: true
Attachment #8676552 - Attachment is obsolete: true
Attachment #8677128 - Flags: review+
Try is green, so I am setting the "checkin-needed" keyword.

Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/66dae447c298
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.