Closed
Bug 1066642
Opened 10 years ago
Closed 9 years ago
IonMonkey MIPS: Do not allocate odd FP registers on Loongson CPU-s.
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: rankov, Assigned: hev)
Details
Attachments
(3 files, 1 obsolete file)
2.08 KB,
patch
|
glandium
:
review-
|
Details | Diff | Splinter Review |
6.80 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
5.84 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
On Loongson CPU-s the odd FP registers behave differently in fp-32 mode than standard MIPS. It is more efficient not to use them than to generate the code to work around the problem.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8488657 -
Flags: review?(mrosenberg)
Reporter | ||
Updated•10 years ago
|
Attachment #8488655 -
Flags: review?(mh+mozilla)
Comment 3•10 years ago
|
||
Comment on attachment 8488657 [details] [diff] [review] Fix-Loongson-fp-patr2.patch Review of attachment 8488657 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/mips/Architecture-mips.h @@ +421,4 @@ > return FloatRegister(index * 2, RegType(kind)); > + return FloatRegister(index, RegType(kind)); > +#else > +# error "Unsupported ABI" I feel like having multiple #errors for !defined(USES_O32_ABI) is kind of redundant, at least within one file. On the other hand, it tells you exactly where you need to make modifications, should you add N32 support. @@ +523,3 @@ > return true; > +#else > +# error "Unsupported ABI" Ditto.
Attachment #8488657 -
Flags: review?(mrosenberg) → review+
Comment 4•10 years ago
|
||
Comment on attachment 8488655 [details] [diff] [review] Fix-Loongson-fp-patr1.patch Review of attachment 8488655 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/configure.in @@ +3251,5 @@ > + AC_MSG_ERROR([Invalid --with-mips-arch-variant value.]) > +fi > + > +AC_SUBST(MIPS_ARCH_MIPS32R2) > +AC_SUBST(MIPS_ARCH_MIPS32R1) You're not using any of those two. Also, this strikes me as very wrong to do a build-time thing, when, for instance, aiui, besides the kernel, running debian on loongson uses the standard mipsel packages. It would be much better to go with runtime detection.
Attachment #8488655 -
Flags: review?(mh+mozilla) → review-
Reporter | ||
Comment 5•10 years ago
|
||
I have done some investigation. Debian packages work on Loongson because debian builds all packages for MIPS2 target. MIPS2 is the least common denominator for most of the MIPS available today. I have made a port for MIPS32r2 and MIPS32r1. It also worked on Loongson3a until support for odd registers was added. This is because of difference between MIPS32r2 and Loongson3a. I will investigate into making this change dynamic.
Assignee | ||
Updated•9 years ago
|
Assignee: branislav.rankov → r
Assignee | ||
Comment 6•9 years ago
|
||
Workaround for Loongson3-series CPU-s, depends on GCC built-in architecture macro.
Attachment #8672270 -
Flags: review?(wtc)
Attachment #8672270 -
Flags: review?(branislav.rankov)
Comment 7•9 years ago
|
||
Comment on attachment 8672270 [details] [diff] [review] 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch Review of attachment 8672270 [details] [diff] [review]: ----------------------------------------------------------------- Hi Heiher, I am not familiar with js, so I am not qualified to review this patch. Sorry!
Attachment #8672270 -
Flags: review?(wtc)
Assignee | ||
Updated•9 years ago
|
Attachment #8672270 -
Flags: review?(arai.unmht)
Comment 8•9 years ago
|
||
Comment on attachment 8672270 [details] [diff] [review] 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch Review of attachment 8672270 [details] [diff] [review]: ----------------------------------------------------------------- I need some hint to complete reviewing. ::: js/src/jit/mips32/Architecture-mips32.h @@ +70,5 @@ > static Code FromName(const char* name); > > static const uint32_t Total = 64; > static const uint32_t TotalDouble = 16; > + static const uint32_t TotalRegisters = 32; Can you add some comment about what this constant actually means? @@ +73,5 @@ > static const uint32_t TotalDouble = 16; > + static const uint32_t TotalRegisters = 32; > +#if defined(_MIPS_ARCH_LOONGSON3A) > + static const uint32_t TotalSingle = 16; > + static const uint32_t Allocatable = 32; The number of allocatable registers is |28 == TotalDouble + TotalSingle - 4| where 4 is for f16(double), f18(double), f16(single), and f18(single), isn't it? > static const SetType NonAllocatableDoubleMask = > ((1ULL << FloatRegisters::f16) | > (1ULL << FloatRegisters::f18)) << 32; > // f16-single and f17-single alias f16-double ... > static const SetType NonAllocatableMask = > NonAllocatableDoubleMask | > (1ULL << FloatRegisters::f16) | > (1ULL << FloatRegisters::f17) | > (1ULL << FloatRegisters::f18) | > (1ULL << FloatRegisters::f19); @@ +199,5 @@ > uint32_t code = i & 31; > uint32_t kind = i >> 5; > return FloatRegister(code, RegType(kind)); > } > // This is similar to FromCode except for double registers on O32. This comment needs update to mention about Loongson. @@ +202,5 @@ > } > // This is similar to FromCode except for double registers on O32. > static FloatRegister FromIndex(uint32_t index, RegType kind) { > #if defined(USES_O32_ABI) > +# if !defined(_MIPS_ARCH_LOONGSON3A) Does |defined(_MIPS_ARCH_LOONGSON3A)| imply |defined(USES_O32_ABI)| ? If not, can you tell me why |defined(_MIPS_ARCH_LOONGSON3A)| branch is not needed for other case? @@ +207,2 @@ > if (kind == Double) > +# endif #ifdef only for |if (...)| line is a little confusing. can this be separated somehow? @@ +209,1 @@ > return FloatRegister(index * 2, RegType(kind)); Looks like |RegType(...)| cast here is redundant. Can we just remove it? @@ +209,3 @@ > return FloatRegister(index * 2, RegType(kind)); > #endif > return FloatRegister(index, RegType(kind)); Here too.
Attachment #8672270 -
Flags: review?(arai.unmht) → feedback+
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8672270 [details] [diff] [review] 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch Review of attachment 8672270 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for review, I will update the patch. ::: js/src/jit/mips32/Architecture-mips32.h @@ +70,5 @@ > static Code FromName(const char* name); > > static const uint32_t Total = 64; > static const uint32_t TotalDouble = 16; > + static const uint32_t TotalRegisters = 32; OK @@ +73,5 @@ > static const uint32_t TotalDouble = 16; > + static const uint32_t TotalRegisters = 32; > +#if defined(_MIPS_ARCH_LOONGSON3A) > + static const uint32_t TotalSingle = 16; > + static const uint32_t Allocatable = 32; Right ;) @@ +199,5 @@ > uint32_t code = i & 31; > uint32_t kind = i >> 5; > return FloatRegister(code, RegType(kind)); > } > // This is similar to FromCode except for double registers on O32. OK @@ +202,5 @@ > } > // This is similar to FromCode except for double registers on O32. > static FloatRegister FromIndex(uint32_t index, RegType kind) { > #if defined(USES_O32_ABI) > +# if !defined(_MIPS_ARCH_LOONGSON3A) This workaround is not needed for other ABIs. @@ +207,2 @@ > if (kind == Double) > +# endif OK @@ +209,1 @@ > return FloatRegister(index * 2, RegType(kind)); OK
Assignee | ||
Comment 10•9 years ago
|
||
Thanks!
Attachment #8672270 -
Attachment is obsolete: true
Attachment #8672270 -
Flags: review?(branislav.rankov)
Attachment #8674717 -
Flags: review?(arai.unmht)
Comment 11•9 years ago
|
||
Comment on attachment 8674717 [details] [diff] [review] 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch Review of attachment 8674717 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :D ::: js/src/jit/mips32/Architecture-mips32.h @@ +53,5 @@ > static Code FromName(const char* name); > > static const uint32_t Total = 64; > static const uint32_t TotalDouble = 16; > + static const uint32_t MaxRegisterId = 32; as asserting |r.id() < FloatRegisters::MaxRegisterId| etc, maximum id is 31. "RegisterIdLimit" or something like that would be better :)
Attachment #8674717 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #11) > Comment on attachment 8674717 [details] [diff] [review] > 0001-IonMonkey-MIPS32-Do-not-allocate-odd-FP-registers-on.patch > > Review of attachment 8674717 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you :D > > ::: js/src/jit/mips32/Architecture-mips32.h > @@ +53,5 @@ > > static Code FromName(const char* name); > > > > static const uint32_t Total = 64; > > static const uint32_t TotalDouble = 16; > > + static const uint32_t MaxRegisterId = 32; > > as asserting |r.id() < FloatRegisters::MaxRegisterId| etc, maximum id is 31. > "RegisterIdLimit" or something like that would be better :) Thanks! ;)
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dd07e888b4f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•