Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

8 years ago
Posted patch (WIP)Add in ICs for ARM (obsolete) — Splinter Review
Bug 707854 added ICs for JSOP_GETPROP to IonMonkey, but only implemented the backend for x86 and x64.  For the most part, the port to ARM is straightforward.  The biggest hitch is IonMonkey's new dynamically allocated pools mean that while assembling code, we don't actually know what the offset of anything is supposed to be until linking is done.

I'm currently in the process of getting the last few bugs out of the patch.
Reporter

Comment 1

8 years ago
Posted patch implement ICs on ARM (obsolete) — Splinter Review
Attachment #586923 - Attachment is obsolete: true
Attachment #587930 - Flags: review?(dvander)
Comment on attachment 587930 [details] [diff] [review]
implement ICs on ARM

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

r=me with some refactoring:

::: js/src/ion/Ion.cpp
@@ +501,4 @@
>  {
>      memcpy(cacheList(), caches, numCaches() * sizeof(IonCache));
>  
> +    masm.resetCounter();

Can we rename this to "beginOffsetComputation" or something?

::: js/src/ion/IonCaches.cpp
@@ +170,5 @@
> +IonCache::updateBaseAddress(IonCode *code, MacroAssembler &masm)
> +{
> +    initialJump_.repoint(code, masm.additionalOffset(initialJump_.offset()));
> +    lastJump_.repoint(code, masm.additionalOffset(lastJump_.offset()));
> +    cacheLabel_.repoint(code, masm.additionalOffset(cacheLabel_.offset()));

If possible, I would try to rearrange this so repoint looks like:

void
CodeLocationJump::repoint(IonCode *code, MacroAssembler &masm)

And then change additionalOffset to be:

ptrdiff_t
AssemblerXYZ::actualOffsetOf(size_t offset) ...

Bonus points if actualOffsetOf takes in a CodeLocationJump instead.

::: js/src/ion/IonMacroAssembler.h
@@ +173,1 @@
>          return CodeOffsetLabel(size());

This function should be moved into MacroAssembler-$(ARCH) to avoid the #ifdef

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +40,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #ifndef jsion_assembler_x86_shared__
>  #define jsion_assembler_x86_shared__
> +#include <cstddef>

What is this for?

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +713,5 @@
> +    void resetCounter() {
> +        curDumpsite = 0;
> +    }
> +    ptrdiff_t poolSizeBefore(ptrdiff_t offset) {
> +        while(curDumpsite < numDumps && poolInfo[curDumpsite].offset <= offset)

Space in between while and (
Attachment #587930 - Flags: review?(dvander) → review+
Comment on attachment 587930 [details] [diff] [review]
implement ICs on ARM

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

::: js/src/ion/IonMacroAssembler.h
@@ +166,5 @@
>              storeCallResult(dest.typedReg());
>      }
>  
>      CodeOffsetLabel labelForPatch() {
> +#if defined(JS_CPU_ARM)

Errr, this is the #ifdef we should remove, in labelForPatch -- splinter did me no favors
Reporter

Comment 4

8 years ago
Fixing some of the nits required a bit of code refactoring, so I'm posting a new patch.  You probably only want to look at the differences between the two patches.
Attachment #587930 - Attachment is obsolete: true
Attachment #590775 - Flags: review?(dvander)
Comment on attachment 590775 [details] [diff] [review]
With a bit more

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

::: js/src/ion/IonMacroAssembler.h
@@ +194,5 @@
>      }
>  
>      CodeOffsetLabel labelForPatch() {
> +#if defined(JS_CPU_ARM)
> +        return CodeOffsetLabel(nextOffset().getOffset());

This should be moved into individual MacroAssembler-$(ARCH) s

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +832,5 @@
>      }
> +
> +    void resetCounter() {}
> +    ptrdiff_t actualOffset(uint8* x) {
> +        return 0;

hrm!

(Also, resetCounter should be more descriptive - maybe prepareFor...Something)
Attachment #590775 - Flags: review?(dvander) → review+
Reporter

Updated

8 years ago
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.