Closed Bug 1423857 Opened 2 years ago Closed 2 years ago

MIPS: Add support for big-endian

Categories

(Core :: JavaScript Engine: JIT, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

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

Details

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

Currently big endian mips is not supported. Build works but a large portion of jit tests fail (over 40%). This is because little endian architecture is assumed in a portion of mips code.



Expected results:

I will add patches that fix JIT for MIPS big endian.
Before you spend a lot of time on this I think you should check with the JS mailing list [1]; we assume little-endian a lot of places.

[1] dev-tech-js-engine-internals@lists.mozilla.org.
Attachment #8936238 - Flags: review?(bbouvier)
Attachment #8936239 - Flags: review?(bbouvier)
These two patches add big endian support to mips, and when used with patches in Bug1406999 only about 10 jit tests still fail. Some of these tests assume little endian architecture. PTAL.
(In reply to Lars T Hansen [:lth] from comment #1)
> Before you spend a lot of time on this I think you should check with the JS
> mailing list [1]; we assume little-endian a lot of places.
> 
> [1] dev-tech-js-engine-internals@lists.mozilla.org.


Thank you, but I had already spent a lot of time on this before creating the bug :)
Comment on attachment 8936238 [details] [diff] [review]
Platform independent changes for mips big endian support

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

Makes sense; so this effectively disables wasm support on little endian architecture, which is sensible since many stubs and paths assume big endian. Thanks for the patch, I'd like to have a second look.

::: js/src/wasm/WasmJS.cpp
@@ +68,4 @@
>      if (!wasm::HaveSignalHandlers())
>          return false;
>  
> +#if !MOZ_LITTLE_ENDIAN

nit: can you use ifdef here?

@@ +68,5 @@
>      if (!wasm::HaveSignalHandlers())
>          return false;
>  
> +#if !MOZ_LITTLE_ENDIAN
> +        return false;

nit: indentation is off by one tab
Attachment #8936238 - Flags: review?(bbouvier) → feedback+
Comment on attachment 8936239 [details] [diff] [review]
Mips changes for big endian support

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

I've got a suggestion to simplify BaseIndex usage, because at the moment it wouldn't be clear to know when to use ToPayload or not. I'd like to have another look.

(to be fully transparent, I'm in a work week this week and won't be much available, and will be in PTO thereafter until January, so feel free to redirect reviews to other people; lth/nbp/jandem would be great reviewers for this code)

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +729,3 @@
>          as_ss(getOddPair(ft), address.base, off2);
>      } else {
> +        ScratchRegisterScope scratch(asMasm());

Great usage of ScratchRegisterScope!

@@ +944,4 @@
>  MacroAssemblerMIPSCompat::loadUnalignedDouble(const wasm::MemoryAccessDesc& access,
>                                                const BaseIndex& src, Register temp, FloatRegister dest)
>  {
> +    MOZ_ASSERT(MOZ_LITTLE_ENDIAN);

general nit: Can you use static_asserts here?

::: js/src/jit/mips32/MacroAssembler-mips32.h
@@ +400,5 @@
>          return ToPayload(Operand(base)).toAddress();
>      }
>  
> +    BaseIndex ToPayload(BaseIndex base) {
> +        return BaseIndex(base.base, base.index, base.scale, base.offset + NUNBOX32_PAYLOAD_OFFSET);

Instead of this, shouldn't the offsets be correct when *constructing* the BaseIndex struct?

@@ +502,4 @@
>  
>      void pushValue(ValueOperand val);
>      void popValue(ValueOperand val);
> +#if MOZ_LITTLE_ENDIAN

general nit: can you use #ifdef in place of #if?
Attachment #8936239 - Flags: review?(bbouvier)
(In reply to Miran Karic from comment #5)
> (In reply to Lars T Hansen [:lth] from comment #1)
> > Before you spend a lot of time on this I think you should check with the JS
> > mailing list [1]; we assume little-endian a lot of places.
> > 
> > [1] dev-tech-js-engine-internals@lists.mozilla.org.
> 
> 
> Thank you, but I had already spent a lot of time on this before creating the
> bug :)

I hear you, but since we Mozilla employees might have to touch or even maintain this code later (not saying we will, but this could happen), and you might want to gather information about what other changes would be needed where, it seems appropriate to ask or at least emit a heads-up that you are working on this, just to see if there are some major blockers I would not be aware of :)
Priority: -- → P5
Attachment #8936238 - Attachment is obsolete: true
Attachment #8937505 - Flags: review?(lhansen)
Attachment #8936239 - Attachment is obsolete: true
Attachment #8937506 - Flags: review?(lhansen)
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Comment on attachment 8936238 [details] [diff] [review]
> Platform independent changes for mips big endian support
> 
> Review of attachment 8936238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense; so this effectively disables wasm support on little endian
> architecture, which is sensible since many stubs and paths assume big
> endian. Thanks for the patch, I'd like to have a second look.
> 
> ::: js/src/wasm/WasmJS.cpp
> @@ +68,4 @@
> >      if (!wasm::HaveSignalHandlers())
> >          return false;
> >  
> > +#if !MOZ_LITTLE_ENDIAN
> 
> nit: can you use ifdef here?

done

> 
> @@ +68,5 @@
> >      if (!wasm::HaveSignalHandlers())
> >          return false;
> >  
> > +#if !MOZ_LITTLE_ENDIAN
> > +        return false;
> 
> nit: indentation is off by one tab

done
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Comment on attachment 8936239 [details] [diff] [review]
> Mips changes for big endian support
> 
> Review of attachment 8936239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've got a suggestion to simplify BaseIndex usage, because at the moment it
> wouldn't be clear to know when to use ToPayload or not. I'd like to have
> another look.
> 
> (to be fully transparent, I'm in a work week this week and won't be much
> available, and will be in PTO thereafter until January, so feel free to
> redirect reviews to other people; lth/nbp/jandem would be great reviewers
> for this code)
> 
> ::: js/src/jit/mips32/MacroAssembler-mips32.cpp
> @@ +729,3 @@
> >          as_ss(getOddPair(ft), address.base, off2);
> >      } else {
> > +        ScratchRegisterScope scratch(asMasm());
> 
> Great usage of ScratchRegisterScope!

I didn't plan to include this in the patch, removed.

> 
> @@ +944,4 @@
> >  MacroAssemblerMIPSCompat::loadUnalignedDouble(const wasm::MemoryAccessDesc& access,
> >                                                const BaseIndex& src, Register temp, FloatRegister dest)
> >  {
> > +    MOZ_ASSERT(MOZ_LITTLE_ENDIAN);
> 
> general nit: Can you use static_asserts here?

Problem is that breaks big endian build. I just want to make sure this function doesn't somehow get executed on big endian because that wouldn't work properly.

> 
> ::: js/src/jit/mips32/MacroAssembler-mips32.h
> @@ +400,5 @@
> >          return ToPayload(Operand(base)).toAddress();
> >      }
> >  
> > +    BaseIndex ToPayload(BaseIndex base) {
> > +        return BaseIndex(base.base, base.index, base.scale, base.offset + NUNBOX32_PAYLOAD_OFFSET);
> 
> Instead of this, shouldn't the offsets be correct when *constructing* the
> BaseIndex struct?

I had a look but I am not sure how this should be implemented. This function is analogous to the one with Operand parameter:
Operand
MacroAssemblerMIPSCompat::ToPayload(Operand base)
{
    return Operand(Register::FromCode(base.base()), base.disp() + PAYLOAD_OFFSET);
}


> 
> @@ +502,4 @@
> >  
> >      void pushValue(ValueOperand val);
> >      void popValue(ValueOperand val);
> > +#if MOZ_LITTLE_ENDIAN
> 
> general nit: can you use #ifdef in place of #if?

Done
Note that #ifdef won't work for MOZ_{LITTLE,BIG}_ENDIAN, because they're always defined to 0/1:

#if MOZ_BIG_ENDIAN
#  define MOZ_LITTLE_ENDIAN 0
#elif MOZ_LITTLE_ENDIAN
#  define MOZ_BIG_ENDIAN 0
#else
#  error "Cannot determine endianness"
#endif

I'm guessing "Done" was written in advance of actually testing the change -- right?  If not, something is Messed Up and I'd like to know precisely what.
(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #13)
> Note that #ifdef won't work for MOZ_{LITTLE,BIG}_ENDIAN, because they're
> always defined to 0/1:
> 
> #if MOZ_BIG_ENDIAN
> #  define MOZ_LITTLE_ENDIAN 0
> #elif MOZ_LITTLE_ENDIAN
> #  define MOZ_BIG_ENDIAN 0
> #else
> #  error "Cannot determine endianness"
> #endif
> 
> I'm guessing "Done" was written in advance of actually testing the change --
> right?  If not, something is Messed Up and I'd like to know precisely what.

Ouch, yes you are right, I didn't test this thoroughly. I did try to build and that works but some tests would fail. The thing is this patch also depends on some other patches (bug 1406999), without them many tests still fail. So I guess I will revert this to #if.
Comment on attachment 8937505 [details] [diff] [review]
Platform independent changes for mips big endian support

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

::: js/src/jit/CacheIR.h
@@ +321,4 @@
>      }
>  
>    private:
> +    uint64_t data_;

I think this probably breaks some aliasing rule...  I don't even understand why you need to make this change.

Anyway, for changes to CacheIR you should get a review from jandem or evilpie.

::: js/src/wasm/WasmJS.cpp
@@ +68,4 @@
>      if (!wasm::HaveSignalHandlers())
>          return false;
>  
> +#ifndef MOZ_LITTLE_ENDIAN

This needs to be '#if !MOZ_LITTLE_ENDIAN' because that macro is always defined, with value 0 or 1.
Attachment #8937505 - Flags: review?(lhansen)
Comment on attachment 8937506 [details] [diff] [review]
Mips changes for big endian support

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

Looks generally good apart from the confusion with MOZ_LITTLE_ENDIAN.

::: js/src/jit/mips32/MacroAssembler-mips32.cpp
@@ +942,4 @@
>  MacroAssemblerMIPSCompat::loadUnalignedDouble(const wasm::MemoryAccessDesc& access,
>                                                const BaseIndex& src, Register temp, FloatRegister dest)
>  {
> +    MOZ_ASSERT(MOZ_LITTLE_ENDIAN);

Note, these should not even have compiled on big-endian platforms if MOZ_LITTLE_ENDIAN were a define to be tested with ifdef.  (They do compile because it's not - it's always defined.)

::: js/src/jit/mips32/MacroAssembler-mips32.h
@@ +37,4 @@
>  static_assert(1 << defaultShift == sizeof(JS::Value), "The defaultShift is wrong");
>  
>  static const uint32_t LOW_32_MASK = (1LL << 32) - 1;
> +#ifdef MOZ_LITTLE_ENDIAN

This must be '#if MOZ_LITTLE_ENDIAN' (ditto elsewhere here and in the other files)
Attachment #8937506 - Flags: review?(lhansen) → review+
(In reply to Lars T Hansen [:lth] from comment #16)
> Comment on attachment 8937506 [details] [diff] [review]
> Mips changes for big endian support
> 
> Review of attachment 8937506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks generally good apart from the confusion with MOZ_LITTLE_ENDIAN.
> 
> ::: js/src/jit/mips32/MacroAssembler-mips32.cpp
> @@ +942,4 @@
> >  MacroAssemblerMIPSCompat::loadUnalignedDouble(const wasm::MemoryAccessDesc& access,
> >                                                const BaseIndex& src, Register temp, FloatRegister dest)
> >  {
> > +    MOZ_ASSERT(MOZ_LITTLE_ENDIAN);
> 
> Note, these should not even have compiled on big-endian platforms if
> MOZ_LITTLE_ENDIAN were a define to be tested with ifdef.  (They do compile
> because it's not - it's always defined.)
> 
> ::: js/src/jit/mips32/MacroAssembler-mips32.h
> @@ +37,4 @@
> >  static_assert(1 << defaultShift == sizeof(JS::Value), "The defaultShift is wrong");
> >  
> >  static const uint32_t LOW_32_MASK = (1LL << 32) - 1;
> > +#ifdef MOZ_LITTLE_ENDIAN
> 
> This must be '#if MOZ_LITTLE_ENDIAN' (ditto elsewhere here and in the other
> files)

Thank you for the review, and sorry for the delay with the new version of these patches. Yes, this change where I put #ifdef instead of #if I did in a hurry per earlier review comments and didn't test thoroughly, now with the latest version it should be ok.

I also found that some tests were failing on asserts in jit/MIR.cpp, so I adjusted these asserts for big endian and now it works properly. Problem here is in usage of union (similar situation is in CacheIR as well), which stores data differently for big endian, I was investigating a different way to implement this but perhaps just changing these asserts is the best solution. I believe c++11 states using a union by storing one type and reading another is undefined behavior, but I'd rather not deal with this if there is no need.
Attachment #8937505 - Attachment is obsolete: true
Attachment #8941471 - Flags: review?(evilpies)
Here are the latest patches. It seems Bug 1425149 breaks mips build, I didn't have time to take a look at it so I tested before this change and only a small number of tests fail for big endian with these patches.
Attachment #8937506 - Attachment is obsolete: true
Attachment #8941473 - Flags: review?(lhansen)
Comment on attachment 8941473 [details] [diff] [review]
Mips changes for big endian support

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

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
@@ +552,4 @@
>  MacroAssemblerMIPSShared::ma_load_unaligned(const wasm::MemoryAccessDesc& access, Register dest, const BaseIndex& src, Register temp,
>                                              LoadStoreSize size, LoadStoreExtension extension)
>  {
> +    MOZ_ASSERT(MOZ_LITTLE_ENDIAN);

I think this one, and the one below, and the ones in MacroAssembler-mips32.cpp, want comments to explain why they are correct.  It does not look like these are only used when the hardware is little-endian.  (If all big-endian MIPS system can handle unrestricted unaligned loads and the mips IsUnaligned() predicate is updated appropriately then I'll take that back, of course.)
Attachment #8941473 - Flags: review?(lhansen) → review+
(In reply to Lars T Hansen [:lth] from comment #20)
> Comment on attachment 8941473 [details] [diff] [review]
> Mips changes for big endian support
> 
> Review of attachment 8941473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp
> @@ +552,4 @@
> >  MacroAssemblerMIPSShared::ma_load_unaligned(const wasm::MemoryAccessDesc& access, Register dest, const BaseIndex& src, Register temp,
> >                                              LoadStoreSize size, LoadStoreExtension extension)
> >  {
> > +    MOZ_ASSERT(MOZ_LITTLE_ENDIAN);
> 
> I think this one, and the one below, and the ones in
> MacroAssembler-mips32.cpp, want comments to explain why they are correct. 
> It does not look like these are only used when the hardware is
> little-endian.  (If all big-endian MIPS system can handle unrestricted
> unaligned loads and the mips IsUnaligned() predicate is updated
> appropriately then I'll take that back, of course.)

These functions are only used by wasm, and wasm requires little endian architecture so I disabled wasm for big endian in the other patch. We could probably make it work by keeping little endian order in memory on big endian architectures but for now I thought best to disable it.

Do you want me to add comments for this next to these asserts?
(In reply to Miran Karic from comment #21)
> (In reply to Lars T Hansen [:lth] from comment #20)
> > Comment on attachment 8941473 [details] [diff] [review]
> > Mips changes for big endian support
> > 
> > Review of attachment 8941473 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> These functions are only used by wasm, and wasm requires little endian
> architecture so I disabled wasm for big endian in the other patch. We could
> probably make it work by keeping little endian order in memory on big endian
> architectures but for now I thought best to disable it.

Fair enough.

> Do you want me to add comments for this next to these asserts?

It's your code, so to speak, but I think a one-line comment, or maybe a string in the MOZ_ASSERT, would be helpful for readers.  Something like "Wasm-only; wasm is disabled on big-endian." maybe.
Sounds good, I added the comment you suggested to the asserts.
Attachment #8941473 - Attachment is obsolete: true
Attachment #8941801 - Flags: review?(lhansen)
Attachment #8941801 - Flags: review?(lhansen) → review+
Attachment #8941471 - Flags: review?(evilpies) → review?(jdemooij)
Attachment #8941471 - Flags: review?(jdemooij) → review+
Attachment #8941471 - Flags: checkin+
Attachment #8941801 - Flags: checkin+
Keywords: checkin-needed
Attachment #8941471 - Flags: checkin+
Attachment #8941801 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3d7ad9a9c2fe
https://hg.mozilla.org/mozilla-central/rev/3b640564e1bd
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee: nobody → miran.karic
You need to log in before you can comment on or make changes to this bug.