Closed Bug 1352077 Opened 5 years ago Closed 5 years ago

Refactor Ion.cpp::IsCallOpcode to be clearer

Categories

(Core :: JavaScript Engine: JIT, enhancement)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement
Priority:
Not set
Severity:
trivial

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

Details

Attachments

(1 file)

Bug 1352077 - Refactor IonBuilder.cpp::IsCallOpcode
59 bytes, text/x-review-board-request
h4writer
: review+
Details 
While working on spreadcall support, I'm trying to generalize certain checks in Ion to avoid bit-rot with all these new call opcodes.

In practice, IsCallOpcode doesn't care about spreadcalls because they don't inline natives calls, (only scripted calls). This is not obvious and potentially changes in the future, so use IsCallPC instead.
Assignee: nobody → tcampbell
Comment on attachment 8852951 [details]
Bug 1352077 - Refactor IonBuilder.cpp::IsCallOpcode

https://reviewboard.mozilla.org/r/125102/#review128410

Clearing review. I think I introduced this function too quickly and apparently this is causing confusion.
To solve this issue can you:
1) Remove "IsCallOpcode" and directly call "isCallPc"
2) In every function calling this add a boolean at the top and test that instead:

// TODO: Support tracking optimizations for inlining a call and regular
// optimization tracking at the same time. Currently just drop the info of the inlining when that happens.
boolean canTrackOptimization = IsCallOpcode(pc);

Thanks for raising the issue!
Attachment #8852951 - Flags: review?(hv1989)
Comment on attachment 8852951 [details]
Bug 1352077 - Refactor IonBuilder.cpp::IsCallOpcode

https://reviewboard.mozilla.org/r/125102/#review128556

Thanks!

::: js/src/jit/IonBuilder.cpp:5782
(Diff revision 2)
>      MOZ_ASSERT(*emitted == false);
>  
> -    if (!IsCallOpcode(JSOp(*pc)))
> +    // TODO: Support tracking optimizations for inlining a call and regular
> +    // optimization tracking at the same time. Currently just drop optimization
> +    // tracking when that happens.
> +    bool canTrackOptimization = IsCallPC(pc);

My mistake. This should be !IsCallPC(pc); here and everywhere this is duplicated

::: js/src/jit/IonBuilder.cpp
(Diff revision 2)
>  IonBuilder::newObjectTryVM(bool* emitted, JSObject* templateObject)
>  {
>      // Emit a VM call.
>      MOZ_ASSERT(JSOp(*pc) == JSOP_NEWOBJECT || JSOp(*pc) == JSOP_NEWINIT);
>  
> -    if (!IsCallOpcode(JSOp(*pc)))

Good find that this is always a non-call opcode
Attachment #8852951 - Flags: review?(hv1989) → review+
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7463511c50ee
Refactor IonBuilder.cpp::IsCallOpcode r=h4writer
https://hg.mozilla.org/mozilla-central/rev/7463511c50ee
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.