IonMonkey: CallVM with Value& and double.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 585851 [details] [diff] [review]
Add Value& and double support to VMFunctions.
Attachment #585851 - Flags: review?(dvander)
(Assignee)

Updated

6 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 709423
Comment on attachment 585851 [details] [diff] [review]
Add Value& and double support to VMFunctions.

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

Nice, thanks for taking this on.

::: js/src/ion/MoveResolver.h
@@ +73,5 @@
>          explicit MoveOperand(const Register &reg) : kind_(REG), code_(reg.code())
>          { }
>          explicit MoveOperand(const FloatRegister &reg) : kind_(FLOAT_REG), code_(reg.code())
>          { }
> +        MoveOperand(const Register &reg, int32 disp, bool ea = false)

Instead of a boolean parameter, please use an enum (you could re-use kind and make it public, or introduce a new one for the constructor).

::: js/src/ion/VMFunctions.h
@@ +54,5 @@
>  };
>  
> +enum ArgProperties {
> +    ArgProp_WordByValue = 0,
> +    ArgProp_DWordByValue = 1,

s/DWord/Double/ in these names - otherwise it will be very confusing with Intel's unique definition for "dword" (being 32-bit). (Also, is it possible to move this enum inside VMFunction?)

@@ +120,5 @@
> +        // Fetch all double-word flags of explicit arguments.
> +        uint32 n =
> +            ((1 << (explicitArgs * 2)) - 1)
> +            & argumentProperties
> +            & 0x55555555;

The bitmask with big constant deserves a short comment.

@@ +130,5 @@
> +        }
> +        return stackSlots;
> +    }
> +
> +    // DWord argument which are passed by copy are taking the space of 2 C

"Double-sized arguments which are passed by value"

@@ +147,5 @@
> +        // Filter out all arguments which are C references from the
> +        // double-word flags.
> +        n = (n & 0x55555555) & ~(n >> 1);
> +
> +        // Add the number of double-word transfered by value. (expect a few)

What does "except a few" mean?

::: js/src/ion/shared/MoveEmitter-x86-shared.cpp
@@ -187,1 @@
>          }

You might want to assert somewhere in the move emitter that effective addresses never participate in a cycle.
Attachment #585851 - Flags: review?(dvander) → review+
(Assignee)

Comment 3

6 years ago
(In reply to David Anderson [:dvander] from comment #2)
> Comment on attachment 585851 [details] [diff] [review]
> Add Value& and double support to VMFunctions.
> 
> Review of attachment 585851 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +120,5 @@
> > +        // Fetch all double-word flags of explicit arguments.
> > +        uint32 n =
> > +            ((1 << (explicitArgs * 2)) - 1)
> > +            & argumentProperties
> > +            & 0x55555555;
> 
> The bitmask with big constant deserves a short comment.

0x5 = 0b0101 => map "DoubleByValue" flags for each Argument properties.

> @@ +147,5 @@
> > +        // Filter out all arguments which are C references from the
> > +        // double-word flags.
> > +        n = (n & 0x55555555) & ~(n >> 1);
> > +
> > +        // Add the number of double-word transfered by value. (expect a few)
> 
> What does "except a few" mean?

"expect a few" means the following loop will is more likely to do 0 iterations than 16. (which is the worst case of this loop)

I'll comment in the code too ;)
(Assignee)

Comment 4

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