Closed Bug 1392606 Opened 7 years ago Closed 7 years ago

Fix MIPS64 simulator build compilation failures

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: miran.karic, Assigned: miran.karic)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch mips64.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.104 Safari/537.36

Steps to reproduce:

MIPS32 simulator build has been fixed in Bug 1329650, here is a patch that fixes MIPS64. It contains the changes for MIPS64 from patches uploaded in the mentioned bug and new fixes.
Attachment #8899821 - Flags: review?(bbouvier)
Comment on attachment 8899821 [details] [diff] [review]
mips64.patch

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

Thanks for the patch, looks good to me. Only one question below and I'd like feedback from somebody else.

Nicolas, can you take a look at Architecture-mips64.h please?

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.h
@@ +282,5 @@
>      OutOfLineBailout(LSnapshot* snapshot, uint32_t frameSize)
>        : snapshot_(snapshot),
>          frameSize_(frameSize)
> +    {
> +        (void)frameSize_;

Why is this needed?

::: js/src/jit/mips64/Simulator-mips64.cpp
@@ +1620,5 @@
> +
> +void
> +Simulator::handleWasmInterrupt()
> +{
> +    MOZ_CRASH("NIY");

Out of curiosity, do you intend to fix those?
Attachment #8899821 - Flags: review?(bbouvier) → feedback?(nicolas.b.pierron)
Comment on attachment 8899821 [details] [diff] [review]
mips64.patch

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

Architecture-mips64.h looks good.  If I recall correctly MIPS64 has float registers which are like x64 xmm registers for (single & double), and not like arm32 vector registers.
Attachment #8899821 - Flags: feedback?(nicolas.b.pierron) → feedback+
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.h
> @@ +282,5 @@
> >      OutOfLineBailout(LSnapshot* snapshot, uint32_t frameSize)
> >        : snapshot_(snapshot),
> >          frameSize_(frameSize)
> > +    {
> > +        (void)frameSize_;
> 
> Why is this needed?

This is what I have taken from a patch by Lars T Hansen in bug 1329650. Not sure if it is needed, my guess is that it silenced some warning. Without this change the simulator build works and I also managed to build for a mips64 board. If you want I can remove it.

> ::: js/src/jit/mips64/Simulator-mips64.cpp
> @@ +1620,5 @@
> > +
> > +void
> > +Simulator::handleWasmInterrupt()
> > +{
> > +    MOZ_CRASH("NIY");
> 
> Out of curiosity, do you intend to fix those?

Yes, but for the moment I will focus on fixing mips32.
(In reply to Miran Karic from comment #3)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> > ::: js/src/jit/mips-shared/CodeGenerator-mips-shared.h
> > @@ +282,5 @@
> > >      OutOfLineBailout(LSnapshot* snapshot, uint32_t frameSize)
> > >        : snapshot_(snapshot),
> > >          frameSize_(frameSize)
> > > +    {
> > > +        (void)frameSize_;
> > 
> > Why is this needed?
> 
> This is what I have taken from a patch by Lars T Hansen in bug 1329650. Not
> sure if it is needed, my guess is that it silenced some warning. Without
> this change the simulator build works and I also managed to build for a
> mips64 board. If you want I can remove it.

Yes please.

> 
> > ::: js/src/jit/mips64/Simulator-mips64.cpp
> > @@ +1620,5 @@
> > > +
> > > +void
> > > +Simulator::handleWasmInterrupt()
> > > +{
> > > +    MOZ_CRASH("NIY");
> > 
> > Out of curiosity, do you intend to fix those?
> 
> Yes, but for the moment I will focus on fixing mips32.

Cool, you might be interested by the work being carried out in bug 1391248 then.
Attached patch mips64-2.patch (obsolete) — Splinter Review
Here is a fixed version of the patch. Yes, I am aware of that work, Dragan and I work for the same company.
Attachment #8899821 - Attachment is obsolete: true
Attachment #8900344 - Flags: review?(bbouvier)
Comment on attachment 8900344 [details] [diff] [review]
mips64-2.patch

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

Looks good, thanks. Can you format your patch so that it is a proper mercurial changeset, with a valid commit message like:

Bug 1392606: Fix MIPS64 simulator build compilation failures; r=bbouvier

(more instructions there: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F )

Once it's done, can you upload it again and tag me for review again, please?
Attachment #8900344 - Flags: review?(bbouvier) → feedback+
Of course, here is it. Thank you!
Attachment #8900344 - Attachment is obsolete: true
Attachment #8900672 - Flags: review?(bbouvier)
Comment on attachment 8900672 [details] [diff] [review]
MIPS64 build fix patch in the correct format

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

Looks good! I'll set the checkin-needed keyword so it's pushed by one of our sheriffs in CI (inbound). Thanks!
Attachment #8900672 - Flags: review?(bbouvier) → review+
Assignee: nobody → miran.karic
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a8455c373c
Fix MIPS64 simulator build compilation failures. r=bbouvier
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34a8455c373c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: