Last Comment Bug 715276 - IonMonkey: CallVM with Value& and double.
: IonMonkey: CallVM with Value& and double.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on:
Blocks: 713526 713693
  Show dependency treegraph
 
Reported: 2012-01-04 12:18 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-01-09 17:48 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add Value& and double support to VMFunctions. (35.74 KB, patch)
2012-01-04 12:18 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Review

Description Nicolas B. Pierron [:nbp] 2012-01-04 12:18:00 PST
Created attachment 585851 [details] [diff] [review]
Add Value& and double support to VMFunctions.
Comment 1 Nicolas B. Pierron [:nbp] 2012-01-04 19:22:25 PST
*** Bug 709423 has been marked as a duplicate of this bug. ***
Comment 2 David Anderson [:dvander] 2012-01-06 13:12:17 PST
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.
Comment 3 Nicolas B. Pierron [:nbp] 2012-01-06 17:05:52 PST
(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 ;)
Comment 4 Nicolas B. Pierron [:nbp] 2012-01-09 17:48:18 PST
https://hg.mozilla.org/projects/ionmonkey/rev/6adb6479446a

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