Closed Bug 893038 Opened 6 years ago Closed 6 years ago

IonMonkey: Re-enable heavyweight and cloned-function inlining

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: djvj, Unassigned)

References

Details

Attachments

(1 file)

Heavyweight function inlining is currently disabled because the callee of inlined frames may get lost if it's not a constant (which happens for heavweight + cloned inlined functions).

This requires keeping the callee alive until after the inlined function's IR graph.  It should be possible to just add a dummy KeepAlive instruction to the return-block of the inlined function, that is otherwise a no-op.. which keeps the callee value alive.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 897572
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 897572
This triggered this build warning in every .cpp file that #includes MIR.h:
{
Warning: -Woverloaded-virtual in /mozilla-central/js/src/ion/MIR.h:   by 'bool js::ion::MForceUse::congruentTo(js::ion::MDefinition* const&) const'
/mozilla-central/js/src/ion/MIR.h:1015:10: warning:   by 'bool js::ion::MForceUse::congruentTo(js::ion::MDefinition* const&) const' [-Woverloaded-virtual]
      bool congruentTo(MDefinition * const &ins) const {
           ^
}
Sorry, I missed the first part:
{
js/src/ion/MIR.h:371:18: warning: 'virtual bool js::ion::MDefinition::congruentTo(js::ion::MDefinition*) const' was hidden [-Woverloaded-virtual]
}

Looks like the type of the function arg is not quite right.
Attached patch followupSplinter Review
This fixes the one mismatched function signature, and fixes the warning on my system.
Attachment #784190 - Flags: review?(kvijayan)
Comment on attachment 784190 [details] [diff] [review]
followup

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

Thanks for identifying and taking care of that!
Attachment #784190 - Flags: review?(kvijayan) → review+
Comment on attachment 784190 [details] [diff] [review]
followup

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

Canceling review on this.  bbouvier also made this exact same patch, on a dedicated bug (bug 900308), and I r+-ed that instead.

Seems more appropriate to push in that bug and close it.

Sorry for the confusion!
Attachment #784190 - Flags: review+
No worries -- glad it's getting fixed!
https://hg.mozilla.org/mozilla-central/rev/133bde40e6bc
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Looks like a typo in LIRGenerator::visitForceUse(MForceUse *ins):

        if (!useBox(lir, 0, ins->input()));  <<< semicolon?
             return false;
Depends on: 900995
(In reply to Douglas Crosher [:dougc] from comment #11)
> Looks like a typo in LIRGenerator::visitForceUse(MForceUse *ins):
> 
>         if (!useBox(lir, 0, ins->input()));  <<< semicolon?
>              return false;

Yeah, that's a rookie mistake :(  Jonco has a patch for it I'm r+-ing.
Duplicate of this bug: 900995
Blocks: 905723
Depends on: 927984
You need to log in before you can comment on or make changes to this bug.