Last Comment Bug 716469 - IonMonkey: Port ICs to ARM
: IonMonkey: Port ICs to ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 01:13 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-01-27 12:23 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(WIP)Add in ICs for ARM (16.34 KB, patch)
2012-01-09 01:13 PST, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Review
implement ICs on ARM (16.32 KB, patch)
2012-01-11 20:01 PST, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Review
With a bit more (18.78 KB, patch)
2012-01-23 10:30 PST, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Review

Description Marty Rosenberg [:mjrosenb] 2012-01-09 01:13:19 PST
Created attachment 586923 [details] [diff] [review]
(WIP)Add in ICs for ARM

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.
Comment 1 Marty Rosenberg [:mjrosenb] 2012-01-11 20:01:24 PST
Created attachment 587930 [details] [diff] [review]
implement ICs on ARM
Comment 2 David Anderson [:dvander] 2012-01-13 14:43:29 PST
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 (
Comment 3 David Anderson [:dvander] 2012-01-13 14:45:09 PST
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
Comment 4 Marty Rosenberg [:mjrosenb] 2012-01-23 10:30:34 PST
Created attachment 590775 [details] [diff] [review]
With a bit more

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.
Comment 5 David Anderson [:dvander] 2012-01-23 16:23:00 PST
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)

Note You need to log in before you can comment on or make changes to this bug.