1175556 - ARM64: Land final, miscellaneous changes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: therest.patch

These are the final changes to the codebase required to get ARM64 building, except for the files added by Bug 1173969. Some parts of Bug 1160672 also snuck in here, as well as a few bugfixes.

All ARM64 code for AsmJS and Ion was removed and replaced with MOZ_CRASH() where appropriate.

These changes are small and isolated, so review should be easy.

Comment 1

[Tom S. (please needinfo tschuster)]

Comment on attachment 8623720 [details] [diff] [review]
therest.patch

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

I really don't think that I am good person to review this.

::: js/src/jit/Registers.h
@@ +44,4 @@
>          Register r = { Encoding(code) };
>          return r;
>      }
> +    MOZ_CONSTEXPR Code code() const {

Why? And do we still have assertion coverage, i.e. the constructor?

::: js/src/jit/arm64/MacroAssembler-arm64.cpp
@@ +6,4 @@
>  
>  #include "jit/arm64/MacroAssembler-arm64.h"
>  
> +// #include "jit/arm64/MoveEmitter-arm64.h"

Remove this line or add a bug.

::: js/src/jit/shared/Assembler-shared.h
@@ +708,5 @@
>  // everywhere. Values are asserted in AsmJSModule.h.
>  static const unsigned AsmJSActivationGlobalDataOffset = 0;
>  static const unsigned AsmJSHeapGlobalDataOffset = sizeof(void*);
> +#if defined(JS_CODEGEN_X64) || defined(JS_CODEGEN_ARM64)
> +static const unsigned AsmJSNaN64GlobalDataOffset = 3 * sizeof(void*);

I have no idea if this is ok or what this means.

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +823,4 @@
>              return;
>          unsigned destOffset = branch.getOffset() + offset;
>          if (offset > 0) {
> +            while ((int)curpool < (int)numDumps_ && poolInfo_[curpool].offset <= (size_t)destOffset) {

Why? Should both be unsigned.

Comment 2

[Sean Stangl [:sstangl]]

(In reply to Tom Schuster [:evilpie] from comment #1)
> I really don't think that I am good person to review this.

At the moment, nobody but me is really able to review changes for ARM64, but we need reviews, so this will have to make do.

> ::: js/src/jit/Registers.h
> @@ +44,4 @@
> >          Register r = { Encoding(code) };
> >          return r;
> >      }
> > +    MOZ_CONSTEXPR Code code() const {
> 
> Why? And do we still have assertion coverage, i.e. the constructor?

To construct constexpr registers. And no, those were converted to constexpr a while ago.

> ::: js/src/jit/arm64/MacroAssembler-arm64.cpp
> @@ +6,4 @@
> >  
> >  #include "jit/arm64/MacroAssembler-arm64.h"
> >  
> > +// #include "jit/arm64/MoveEmitter-arm64.h"
> 
> Remove this line or add a bug.

This depends on Bug 1173969 to provide the file.

> 
> ::: js/src/jit/shared/Assembler-shared.h
> @@ +708,5 @@
> >  // everywhere. Values are asserted in AsmJSModule.h.
> >  static const unsigned AsmJSActivationGlobalDataOffset = 0;
> >  static const unsigned AsmJSHeapGlobalDataOffset = sizeof(void*);
> > +#if defined(JS_CODEGEN_X64) || defined(JS_CODEGEN_ARM64)
> > +static const unsigned AsmJSNaN64GlobalDataOffset = 3 * sizeof(void*);
> 
> I have no idea if this is ok or what this means.

That's OK: I don't either! It's also probably wrong. It's a relic from Marty that's necessary for the file to compile. AsmJS support doesn't work yet (it's MOZ_CRASH()), so it doesn't really matter, and I'll fix it when that time comes.

With the wrong value everything will break anyway.

> ::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
> @@ +823,4 @@
> >              return;
> >          unsigned destOffset = branch.getOffset() + offset;
> >          if (offset > 0) {
> > +            while ((int)curpool < (int)numDumps_ && poolInfo_[curpool].offset <= (size_t)destOffset) {
> 
> Why? Should both be unsigned.

You would think so, but internally IonAssemblerBufferWithConstantPools uses ints, and it was requested in the relevant patch to just standardize on something. Int involved the least amount of code change. Can pick a different type in a different bug.

Comment 3

[Tom S. (please needinfo tschuster)]

(In reply to Sean Stangl [:sstangl] from comment #2)
> (In reply to Tom Schuster [:evilpie] from comment #1)
> > I really don't think that I am good person to review this.
> 
> At the moment, nobody but me is really able to review changes for ARM64, but
> we need reviews, so this will have to make do.
> 
This situation is always unfortunate.
> > ::: js/src/jit/Registers.h
> > @@ +44,4 @@
> > >          Register r = { Encoding(code) };
> > >          return r;
> > >      }
> > > +    MOZ_CONSTEXPR Code code() const {
> > 
> > Why? And do we still have assertion coverage, i.e. the constructor?
> 
> To construct constexpr registers. And no, those were converted to constexpr
> a while ago.
> 
> > ::: js/src/jit/arm64/MacroAssembler-arm64.cpp
> > @@ +6,4 @@
> > >  
> > >  #include "jit/arm64/MacroAssembler-arm64.h"
> > >  
> > > +// #include "jit/arm64/MoveEmitter-arm64.h"
> > 
> > Remove this line or add a bug.
> 
> This depends on Bug 1173969 to provide the file.
> 
> > 
> > ::: js/src/jit/shared/Assembler-shared.h
> > @@ +708,5 @@
> > >  // everywhere. Values are asserted in AsmJSModule.h.
> > >  static const unsigned AsmJSActivationGlobalDataOffset = 0;
> > >  static const unsigned AsmJSHeapGlobalDataOffset = sizeof(void*);
> > > +#if defined(JS_CODEGEN_X64) || defined(JS_CODEGEN_ARM64)
> > > +static const unsigned AsmJSNaN64GlobalDataOffset = 3 * sizeof(void*);
> > 
> > I have no idea if this is ok or what this means.
> 
> That's OK: I don't either! It's also probably wrong. It's a relic from Marty
> that's necessary for the file to compile. AsmJS support doesn't work yet
> (it's MOZ_CRASH()), so it doesn't really matter, and I'll fix it when that
> time comes.
> 
> With the wrong value everything will break anyway.
> 
What about the defined(JS_CODEGEN_X64) that seems like a unwanted change.
> > ::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
> > @@ +823,4 @@
> > >              return;
> > >          unsigned destOffset = branch.getOffset() + offset;
> > >          if (offset > 0) {
> > > +            while ((int)curpool < (int)numDumps_ && poolInfo_[curpool].offset <= (size_t)destOffset) {
> > 
> > Why? Should both be unsigned.
> 
> You would think so, but internally IonAssemblerBufferWithConstantPools uses
> ints, and it was requested in the relevant patch to just standardize on
> something. Int involved the least amount of code change. Can pick a
> different type in a different bug.
Do you have another patch that changes this https://dxr.mozilla.org/mozilla-central/source/js/src/jit/shared/IonAssemblerBufferWithConstantPools.h#378 ? I at least prefer casts like int(curpool).

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efe724729178

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddf0a252b08 for mass build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=11103539&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=11103540&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=11103533&repo=mozilla-inbound

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9e24957f2d

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f9e24957f2d