925733 - don't use JS_BITS_PER_WORD_LOG2 when compiling jit caches

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: don't use JS_BITS_PER_WORD_LOG2 when compiling jit caches

I don't really understand what the code is trying to do, but I tried to explain
how the code is getting simplified in the comments.  If somebody could provide
an explanation for why we're scaling by ScaleFromElemWidth(sizeof(size_t)), I
think that would improve the comment(s) significantly.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 815836 [details] [diff] [review]
don't use JS_BITS_PER_WORD_LOG2 when compiling jit caches

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

::: js/src/jit/BaselineIC.cpp
@@ -4499,3 @@
>      masm.movePtr(idxReg, tempReg);
> -    masm.rshiftPtr(Imm32(JS_BITS_PER_WORD_LOG2), tempReg);
> -    masm.loadPtr(BaseIndex(scratchReg, tempReg, ScaleFromElemWidth(sizeof(size_t))), scratchReg);

What this does is: load a word stored at scratchReg + (tempReg * sizeof(size_t))

If you change it to rshiftPtr(3) + TimesOne you calculate the byte index but then load a word from it. This could read invalid memory if you access the last byte. It could work if you also used load8 or something instead of loadPtr but that's probably slower than loading a word.

I think we should change the code to something like:

const uint32_t shift = FloorLog2(sizeof(size_t));
static_assert(shift == 5 || shift == 6);

masm.move32(idxReg, tempReg);
masm.rshift32(Imm32(shift), tempReg);
masm.loadPtr(BaseIndex(scratchReg, tmepReg, ScaleFromElemWidth(sizeof(size_t))), scratchReg);

If you feel really adventurous, you could combine the loadPtr and the branchPtr that follows it into a single branchPtr, but it probably doesn't matter much here.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 815836 [details] [diff] [review]
> don't use JS_BITS_PER_WORD_LOG2 when compiling jit caches
> 
> Review of attachment 815836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/BaselineIC.cpp
> @@ -4499,3 @@
> >      masm.movePtr(idxReg, tempReg);
> > -    masm.rshiftPtr(Imm32(JS_BITS_PER_WORD_LOG2), tempReg);
> > -    masm.loadPtr(BaseIndex(scratchReg, tempReg, ScaleFromElemWidth(sizeof(size_t))), scratchReg);
> 
> What this does is: load a word stored at scratchReg + (tempReg *
> sizeof(size_t))
> 
> If you change it to rshiftPtr(3) + TimesOne you calculate the byte index but
> then load a word from it. This could read invalid memory if you access the
> last byte. It could work if you also used load8 or something instead of
> loadPtr but that's probably slower than loading a word.

Oh, bleh, I forgot about the bottom bits that were getting shifted off.  You're right.

> I think we should change the code to something like:
> 
> const uint32_t shift = FloorLog2(sizeof(size_t));
> static_assert(shift == 5 || shift == 6);
> 
> masm.move32(idxReg, tempReg);
> masm.rshift32(Imm32(shift), tempReg);
> masm.loadPtr(BaseIndex(scratchReg, tmepReg,
> ScaleFromElemWidth(sizeof(size_t))), scratchReg);

Guess that gets us to the same place.  I'll try that instead.

Comment 4

[Nathan Froyd [:froydnj]]

Also, impressively, forgetting about those extra bits did not cause test failures...

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: don't use JS_BITS_PER_WORD_LOG2 for jit caches

Second revision, this time with FloorLog2 usage.  The parenthesis, IIRC, are needed
for MSVC, which can get unhappy about expressions as template parameters.  Needed
to use JS_ASSERT instead of JS_STATIC_ASSERT, since the shift value won't be a
compile-time constant--at least at the point static asserts are checked.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 815906 [details] [diff] [review]
don't use JS_BITS_PER_WORD_LOG2 for jit caches

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

Nice, thanks! Just some pre-existing nits while you're here.

We load the value as int32 so we should use move32/rshift32 instead of movePtr/rshiftPtr.

::: js/src/jit/BaselineIC.cpp
@@ +4498,5 @@
>      // Load deletedBits bitArray pointer into scratchReg
>      masm.loadPtr(Address(argData, offsetof(ArgumentsData, deletedBits)), scratchReg);
>  
>      // In tempReg, calculate index of word containing bit: (idx >> logBitsPerWord)
>      masm.movePtr(idxReg, tempReg);

Nit: move32 while you're here.

@@ +4501,5 @@
>      // In tempReg, calculate index of word containing bit: (idx >> logBitsPerWord)
>      masm.movePtr(idxReg, tempReg);
> +    const uint32_t shift = mozilla::tl::FloorLog2<(sizeof(size_t) * JS_BITS_PER_BYTE)>::value;
> +    JS_ASSERT(shift == 5 || shift == 6);
> +    masm.rshiftPtr(Imm32(shift), tempReg);

Nit: rshift32 while you're here.

::: js/src/jit/IonCaches.cpp
@@ +3101,5 @@
>  
>      // In tempReg, calculate index of word containing bit: (idx >> logBitsPerWord)
> +    const uint32_t shift = FloorLog2<(sizeof(size_t) * JS_BITS_PER_BYTE)>::value;
> +    JS_ASSERT(shift == 5 || shift == 6);
> +    masm.rshiftPtr(Imm32(shift), indexReg);

Nit: rshift32

Comment 7

[Jan de Mooij [:jandem]]

Oh rshift32 doesn't exist yet, looks like. Feel free to land the patch without fixing these nits.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/612f4837513a