807879 - endian-sensitive things for MIPS should use MIPS-specific macros

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

...not the NSPR ones.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: don't use IS_LITTLE_ENDIAN for MIPS-specific things; there are MIPS-specific macros to use

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 677634 [details] [diff] [review]
don't use IS_LITTLE_ENDIAN for MIPS-specific things; there are MIPS-specific macros to use

OK, so it's not all JS...close enough.  Picking glandium as the likeliest reviewer with experience in this area.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 677634 [details] [diff] [review]
don't use IS_LITTLE_ENDIAN for MIPS-specific things; there are MIPS-specific macros to use

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

r+, but what exactly are you trying to solve? I mean, I understand we want to get rid of nspr header uses, but there *is* a need for endianness macros. We might as well use such macros, wherever they're defined.

::: js/src/methodjit/NunboxAssembler.h
@@ -469,5 @@
>  #elif defined JS_CPU_ARM
>          // Yes, we are backwards from SPARC.
>          fastStoreDouble(srcDest, dataReg, typeReg);
>  #elif defined JS_CPU_MIPS
> -#if defined(IS_LITTLE_ENDIAN)

in js, IS_LITTLE_ENDIAN is defined in jscpucfg.h.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #3)
> r+, but what exactly are you trying to solve? I mean, I understand we want
> to get rid of nspr header uses, but there *is* a need for endianness macros.
> We might as well use such macros, wherever they're defined.

Sure; the proposed patch for mfbt/Endian.h has its own endianness macros.

...however, Waldo is skeptical of the need for them.  I think there are a couple of places in the tree which really do need them, but the first iteration of mfbt/Endian.h may well end up going in without them.  I think it's a little clearer to use the platform-specific stuff in platform-specific code.

We'll ask Waldo, see what he thinks.  Be nice to have a JS person's input on JS engine changes anyway.

Comment 5

[Jeff Walden [:Waldo]]

I think we want to have, and use, exactly one way to test for any particular thing.  Then that that one way is the way everyone uses, and everyone will understand upon reading it.  This makes it easier for a non-ARM person to make slight changes to ARM code if necessary, and for tree-wide changes to proceed with fewer roadbumps.  So I don't think we should be using _MIPSEL or _MIPSEB here.  (Doesn't help they're not super-searchable, either.)

Regarding endianness macros, in the cases that really do need them, my vague idea is to have people write templates with big-endian and little-endian modes both, then have Endian.h pick the one to perform.  Super-vague sketch, don't bikeshed me bro:

struct Behavior
{
    void littleEndian() {
        fastStoreDouble(srcDest, dataReg, typeReg);
    }
    void bigEndian() {
        fastStoreDouble(srcDest, typeReg, dataReg);
    }
};

  // Then you'd create a Behavior, and this would call one or the other method.
  static void NativeEndian::perform(Behavior behavior);

Maybe it won't be possible in the end, I dunno.  asm might be a tricky case for this, if it's even possible.  I'd just really like to avoid people reimplementing already-provided functionality, and providing macros seems more likely to cause that to happen.

Comment 6

[Nathan Froyd [:froydnj]]

OK, we'll use The One True Mechanism for the MIPS stuff, then.