1392606 - Fix MIPS64 simulator build compilation failures

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Miran Karic]

Attachment: mips64.patch

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.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 2

[Nicolas B. Pierron [:nbp]]

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.

Comment 3

[Miran Karic]

(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.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(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.

Comment 5

[Miran Karic]

Attachment: mips64-2.patch

Here is a fixed version of the patch. Yes, I am aware of that work, Dragan and I work for the same company.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 7

[Miran Karic]

Attachment: MIPS64 build fix patch in the correct format

Of course, here is it. Thank you!

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

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!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a8455c373c
Fix MIPS64 simulator build compilation failures. r=bbouvier

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34a8455c373c