don't use JS_BITS_PER_WORD_LOG2 when compiling jit caches

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
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.
Attachment #815836 - Flags: review?(jdemooij)
Blocks: 781171
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.
Attachment #815836 - Flags: review?(jdemooij)
(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.
Also, impressively, forgetting about those extra bits did not cause test failures...
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.
Attachment #815836 - Attachment is obsolete: true
Attachment #815906 - Flags: review?(jdemooij)
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
Attachment #815906 - Flags: review?(jdemooij) → review+
Oh rshift32 doesn't exist yet, looks like. Feel free to land the patch without fixing these nits.
https://hg.mozilla.org/mozilla-central/rev/612f4837513a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.