1050254 - OdinMonkey: place NaN in the global data and load from here to free the NANReg.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

The ARM and MIPS backends pin a float register to hold NaN so that NaN can be loaded quickly for out-of-bounds float array reads. These backends also pin a general register to hold the a pointer to the global data and if NaN is placed near the start of the global data and then it can be loaded in a single instruction. This out-of-bounds load is a slow path so it would not be a concern if it were a little slower. Freeing a float register should help performance. It's also one less register to initialize when control passes into, or returns to, asm.js code.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Place NaN in the global data and load from here to free the NANReg.

Would you consider this change for the MIPS backend too?

Comment 2

[Branislav Rankov [:rankov]]

Comment on attachment 8469266 [details] [diff] [review]
Place NaN in the global data and load from here to free the NANReg.

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

This looks good to me. Land it :)

Comment 3

[Luke Wagner [:luke]]

Good idea!

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8469266 [details] [diff] [review]
Place NaN in the global data and load from here to free the NANReg.

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

Great!

::: js/src/asmjs/AsmJSModule.h
@@ +1080,5 @@
>      uint8_t *&heapDatum() const {
>          JS_ASSERT(isFinished());
>          return *(uint8_t**)(globalData() + heapGlobalDataOffset());
>      }
> +    static unsigned nanGlobalDataOffset() {

How about nan64GlobalDataOffset (and likewise for globalNaN)?

@@ +1081,5 @@
>          JS_ASSERT(isFinished());
>          return *(uint8_t**)(globalData() + heapGlobalDataOffset());
>      }
> +    static unsigned nanGlobalDataOffset() {
> +        return heapGlobalDataOffset() + sizeof(void*);

Can you add asserts here and in the 32 case that AsmJSNaNGlobalDataOffset == heapGlobalDataOffset() + sizeof(void*) and then statically assert that AsmJSNaNGlobalDataOffset is a multiple of sizeof(double)?

@@ +1083,5 @@
>      }
> +    static unsigned nanGlobalDataOffset() {
> +        return heapGlobalDataOffset() + sizeof(void*);
> +    }
> +    double *globalNaN() const {

How about return double& analogous to heapDatum()?

::: js/src/jit/shared/Assembler-shared.h
@@ +654,5 @@
>  static_assert(sizeof(AsmJSFrame) == 2 * sizeof(void*), "?!");
>  static const uint32_t AsmJSFrameBytesAfterReturnAddress = sizeof(void*);
>  
>  // A hoisting of AsmJSModule::activationGlobalDataOffset that avoids #including
>  // AsmJSModule everywhere.

Can you update this comment to say "A hoisting of constants that would otherwise require #including AsmJSModule.h everywhere. Values are asserted in AsmJSModule.h."?

Comment 5

[Douglas Crosher [:dougc]]

(In reply to Luke Wagner [:luke] from comment #4)
> Comment on attachment 8469266 [details] [diff] [review]
> Place NaN in the global data and load from here to free the NANReg.

Thank you for the feedback, this helped clean it up.

> > +    static unsigned nanGlobalDataOffset() {
> 
> How about nan64GlobalDataOffset (and likewise for globalNaN)?

Done.
 
> > +    static unsigned nanGlobalDataOffset() {
> > +        return heapGlobalDataOffset() + sizeof(void*);
> 
> Can you add asserts here and in the 32 case that AsmJSNaNGlobalDataOffset ==
> heapGlobalDataOffset() + sizeof(void*) and then statically assert that
> AsmJSNaNGlobalDataOffset is a multiple of sizeof(double)?

Done.

> @@ +1083,5 @@
> >      }
> > +    static unsigned nanGlobalDataOffset() {
> > +        return heapGlobalDataOffset() + sizeof(void*);
> > +    }
> > +    double *globalNaN() const {
> 
> How about return double& analogous to heapDatum()?

These methods were only used to initialize the NaN slots so have been replaced with an initGlobalNaN() method. This avoids making these pointers public. The requested run time assertions have been placed in this initialization function.

> >  // A hoisting of AsmJSModule::activationGlobalDataOffset that avoids #including
> >  // AsmJSModule everywhere.
> 
> Can you update this comment to say "A hoisting of constants that would
> otherwise require #including AsmJSModule.h everywhere. Values are asserted
> in AsmJSModule.h."?

Done.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Place NaN in the global data and load from here to free the NANReg.

Address reviewer feedback. Carrying forward r+.

Comment 7

[Douglas Crosher [:dougc]]

https://tbpl.mozilla.org/?tree=Try&rev=a58f2cb72020

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa9d39c6193

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8aa9d39c6193