IonMonkey: define() should assert it's not used with call instructions

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

7 years ago
Posted patch PatchSplinter Review
Somebody on IRC was hitting an assert because the code was using define instead of defineVMReturn. The attached patch makes define and defineBox assert !ins->isCall(). defineVMReturn already asserts ins->isCall().
Attachment #686118 - Flags: review?(dvander)
One question: the code in question is for the par. exec. mode, which must not use actual VM calls (since the VM is not thread-safe).  It seems though that |defineVMReturn()| is fairly generic and not really specific to VM calls.  Perhaps we could combine |defineVMReturn()| and |defineReturn()| to something which takes a |MIRType| argument specifying the return type?  It seems (to me) like both are overly specific, or at the least that there is a common underlying routine (|defineCallReturn(LInstructionHelper<Defs, Ops, Temps> *lir, MIRType t)| that they could both be based on.

Certainly I'd rather not have VM in the name, as it suggests that the code is only used by VM operations.
Per discussion on IRC with :jandem, it might also be better to just remove |defineReturn| and rename |defineVMReturn| to |defineReturn|.  I was thinking that perhaps the return type of the MIR and the function would not necessarily be the same, but I guess that this is actually not so, since |defineReturn()| is always specific to specifying the register used for the result of the MIR.
Assignee

Comment 3

7 years ago
(In reply to Niko Matsakis from comment #2)
> Per discussion on IRC with :jandem, it might also be better to just remove
> |defineReturn| and rename |defineVMReturn| to |defineReturn|.

dvander, any thoughts on this? I don't think we need both defineReturn and defineVMReturn.
Comment on attachment 686118 [details] [diff] [review]
Patch

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

Yeah, these names are confusing, would be good to simplify them.
Attachment #686118 - Flags: review?(dvander) → review+
Assignee

Comment 6

7 years ago
Posted patch Part 2: Rename defineVMReturn (obsolete) — Splinter Review
Removes defineReturn and renames defineVMReturn to defineReturn.
Attachment #686480 - Flags: review?(dvander)
Assignee

Comment 7

7 years ago
Attachment #686480 - Attachment is obsolete: true
Attachment #686480 - Flags: review?(dvander)
Attachment #686484 - Flags: review?(dvander)
Attachment #686484 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/6e341bb69bcf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.