Make it harder to accidentally forget implementing opcodes in Ionmonkey

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
Created attachment 8874960 [details] [diff] [review]
Warn about missing opcodes in IonMonkey

Right now it's pretty easy and not very obvious when a new feature isn't implemented in IonMonkey. By removing the default case we force people to spell out the missing feature and can warn them.
Attachment #8874960 - Flags: review?(tcampbell)
(Assignee)

Updated

6 months ago
Assignee: nobody → evilpies
Comment on attachment 8874960 [details] [diff] [review]
Warn about missing opcodes in IonMonkey

I like this idea. Rebase once Bug 1169743 lands. Also, I like Jan's idea of doing for baseline as well. We are always going to have exceptions, but centralizing the lists as well as having tracking bugs will be nice for both. Clearing review for now.
Attachment #8874960 - Flags: review?(tcampbell)
Comment on attachment 8874960 [details] [diff] [review]
Warn about missing opcodes in IonMonkey

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

::: js/src/jit/IonBuilder.cpp
@@ +1767,5 @@
>          // If this ever changes, change what JSOP_GIMPLICITTHIS does too.
>          pushConstant(UndefinedValue());
>          return Ok();
>  
> +      case JSOP_IFNE:

nit: Add a comment about control flow opcodes, and where they should be handled.

@@ +2400,5 @@
> +
> +        // === !! WARNING WARNING WARNING !! ===
> +        // Do you really want to sacrifice performance by not implementing this
> +        // operation in the optimizing compiler?
> +        // Please get the appropiate approval!

I think we can remove this last statement.
(Assignee)

Comment 3

5 months ago
Created attachment 8882894 [details] [diff] [review]
v2 - Make the compiler warn about missing opcodes in Ion/Baseline
Attachment #8874960 - Attachment is obsolete: true
Attachment #8882894 - Flags: review?(tcampbell)
Comment on attachment 8882894 [details] [diff] [review]
v2 - Make the compiler warn about missing opcodes in Ion/Baseline

Great. I think this covers the feedback others have given too.
Attachment #8882894 - Flags: review?(tcampbell) → review+

Comment 5

5 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6e75c92bda
Make the compiler warn about missing opcodes in Ion/Baseline. r=tcampbell

Comment 6

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ef6e75c92bda
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.