IonMonkey: visitCallGeneric() should be platform-independent.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sstangl, Assigned: sstangl)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Attachment #584678 - Flags: review?(mrosenberg)
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.
Attachment #584678 - Flags: review?(mrosenberg) → review+
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.
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/0371557c1ec4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.