Last Comment Bug 710948 - IonMonkey: visitCallGeneric() should be platform-independent.
: IonMonkey: visitCallGeneric() should be platform-independent.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Sean Stangl [:sstangl]
:
Mentors:
Depends on: 708441 710130
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-14 18:02 PST by Sean Stangl [:sstangl]
Modified: 2011-12-29 15:55 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make visitCallGeneric() platform-independent, v1 (28.70 KB, patch)
2011-12-28 17:54 PST, Sean Stangl [:sstangl]
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2011-12-14 18:02:45 PST
The visitCallGeneric() function currently has a separate implementation on ARM. The logic is very nearly the same as on x86/x86_64: with a small amount of effort, we can abstract away the differences and have only a single implementation, which is obviously desirable.

We require ARM CallVM support to do this, since bug 708441 uses CallVM from visitCallGeneric() to handle uncompiled/native functions. Once that lands we can merge.

There are probably a few other code generator functions that also should be merged.
Comment 1 Sean Stangl [:sstangl] 2011-12-28 17:54:02 PST
Created attachment 584678 [details] [diff] [review]
Make visitCallGeneric() platform-independent, v1

First attempt at evicting visitCallGeneric(). Appears to pass tests without being too whiny. This brings ARM up to the x86/x64 level of call support, finally.
Comment 2 Marty Rosenberg [:mjrosenb] 2011-12-28 19:19:44 PST
Comment on attachment 584678 [details] [diff] [review]
Make visitCallGeneric() platform-independent, v1

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

You should file a bug about load16.

::: js/src/ion/CodeGenerator.cpp
@@ +494,5 @@
> +        masm.bind(&rejoin);
> +    }
> +
> +    // Increment to remove IonFramePrefix; decrement to fill FrameSizeClass.
> +    int prefixGarbage = 2 * sizeof(void *);

this number should be 3 * sizeof(void *) on arm.  I think  sizeof(IonJSFrameLayout) - sizeof(void*) is a platform independent way of calculating this.  I have no clue how this code is not throwing assertion failures left and right

::: js/src/ion/LIR-Common.h
@@ -245,5 @@
>                   const LDefinition &tmpobjreg)
>        : argslot_(argslot)
>      {
>          setOperand(0, func);
> -        setTemp(0, token);

I am concerned about the disappearance of a temp register, I assume you found a way to multiplex the existing two temp registers in order to get the same effect.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1007,5 @@
> +
> +void
> +MacroAssemblerARMCompat::callIon(const Register &callee)
> +{
> +    ma_callIon(callee);

This should update framePushed, since the net effect is sp+=4.
Comment 3 Marty Rosenberg [:mjrosenb] 2011-12-29 08:44:41 PST
Just noticed that the |visitCallGeneric()| still has two calls to |callIon()|, which arguably is wrong.  If you don't fix it before you push, I have it fixed locally.
Comment 4 Sean Stangl [:sstangl] 2011-12-29 15:44:52 PST
(In reply to Marty Rosenberg [:Marty] from comment #2)
> I am concerned about the disappearance of a temp register, I assume you
> found a way to multiplex the existing two temp registers in order to get the
> same effect.

Yes. Also eliminated 2 unnecessary movePtr instructions in the process.
Comment 5 Sean Stangl [:sstangl] 2011-12-29 15:55:55 PST
http://hg.mozilla.org/projects/ionmonkey/rev/0371557c1ec4

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