893038 - IonMonkey: Re-enable heavyweight and cloned-function inlining

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

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.

Comment 3

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/133bde40e6bc

Comment 4

[Daniel Holbert [:dholbert]]

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 {
           ^
}

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: followup

This fixes the one mismatched function signature, and fixes the warning on my system.

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 784190 [details] [diff] [review]
followup

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

Thanks for identifying and taking care of that!

Comment 8

[Kannan Vijayan [:djvj]]

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!

Comment 9

[Daniel Holbert [:dholbert]]

No worries -- glad it's getting fixed!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/133bde40e6bc

Comment 11

[Douglas Crosher [:dougc]]

Looks like a typo in LIRGenerator::visitForceUse(MForceUse *ins):

        if (!useBox(lir, 0, ins->input()));  <<< semicolon?
             return false;

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26f3f4a5d5f4

Comment 13

[Kannan Vijayan [:djvj]]

(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.