Closed
Bug 715276
Opened 12 years ago
Closed 12 years ago
IonMonkey: CallVM with Value& and double.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file)
35.74 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #585851 -
Flags: review?(dvander)
Assignee | ||
Updated•12 years ago
|
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
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 ®) : kind_(REG), code_(reg.code()) > { } > explicit MoveOperand(const FloatRegister ®) : kind_(FLOAT_REG), code_(reg.code()) > { } > + MoveOperand(const Register ®, 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•12 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•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/6adb6479446a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•