File moarvm_wrong_value_after_multi_level_inlining.diff of Package moarvm

Overview Repositories Revisions Requests Users Attributes Meta

File moarvm_wrong_value_after_multi_level_inlining.diff of Package moarvm (Revision f7235d6f5056257de819d525bad33228)

Currently displaying revision f7235d6f5056257de819d525bad33228 , Show latest

commit 059c013b09443c514a5410dc44667b9c5976cf74
Author: Stefan Seifert <nine@detonation.org>
Date:   Sun Mar 6 18:47:37 2022 +0100

Fix frame walker finding wrong value after multi level inlining
    
    The frame walker expects a spesh cand's inlinees to be sorted depth first. This
    has the benefit of the walker being able to just iterate over the inlinees and
    take the first one that covers the current bytecode position.
    
    In the failing case we'd inlined Map.keys which is just:
    Seq.new(Rakudo::Iterator.Mappy-keys(self)). Originally the spesh cand for keys
    did not have any inlines, so none could get stacked up front. After inlining we
    ran optimize_bb over the inlined graph to get a more optimized version.
    optimize_bb would then encounter those dispatches, try to optimize them and
    succeed at inlining. But those inlinees got added to the table after the
    original inlinee for keys. This caused the frame walker to think that we're
    a frame out of the actual one, which made it return the wrong value for
    ::?CLASS
    
    This is acutally a new-disp regression. Commit bc98cb9467ab2d5d408dff841f39ae87
    originally introduced optimization of inlined graphs after inlining but
    intentionally did not optimize calls in the inlined graph to avoid the problem
    described above. When the code got translated for new-disp, this safety measure
    was omitted. The fix is to simply re-instate the inline-blocker (but without
    blocking the optimization to pick a spesh cand).
    
    A possible future optimization could be to allow for inlining but be smarter
    about allocating the inline indexes.
    
    Fixes Rakudo issue #4655

diff --git a/src/spesh/optimize.c b/src/spesh/optimize.c
index aba5c2599..ca229d0dc 100644
--- a/src/spesh/optimize.c
+++ b/src/spesh/optimize.c
@@ -1544,7 +1544,10 @@ static void optimize_runbytecode(MVMThreadContext *tc, MVMSpeshGraph *g, MVMSpes
         char *no_inline_reason = NULL;
         const MVMOpInfo *no_inline_info = NULL;
         MVMuint32 effective_size;
-        MVMSpeshGraph *inline_graph = MVM_spesh_inline_try_get_graph(tc, g,
+        /* Do not try to inline calls from inlined basic blocks! Otherwise the new inlinees would
+         * get added to the inlines table after the original inlinee which they are nested in and
+         * the frame walker would find the outer inlinee first, giving wrong results */
+        MVMSpeshGraph *inline_graph = bb->inlined ? NULL : MVM_spesh_inline_try_get_graph(tc, g,
             target_sf, target_sf->body.spesh->body.spesh_candidates[spesh_cand],
             ins, &no_inline_reason, &effective_size, &no_inline_info);
         log_inline(tc, g, target_sf, inline_graph, effective_size, no_inline_reason,
@@ -1597,7 +1600,10 @@ static void optimize_runbytecode(MVMThreadContext *tc, MVMSpeshGraph *g, MVMSpes
             }
         }
     }
-    else if (target_sf->body.bytecode_size < MVM_spesh_inline_get_max_size(tc, target_sf)) {
+    /* Do not try to inline calls from inlined basic blocks! Otherwise the new inlinees would
+     * get added to the inlines table after the original inlinee which they are nested in and
+     * the frame walker would find the outer inlinee first, giving wrong results */
+    else if (!bb->inlined && target_sf->body.bytecode_size < MVM_spesh_inline_get_max_size(tc, target_sf)) {
         /* Consider producing a candidate to inline. */
         char *no_inline_reason = NULL;
         const MVMOpInfo *no_inline_info = NULL;

Places

File moarvm_wrong_value_after_multi_level_inlining.diff of Package moarvm (Revision f7235d6f5056257de819d525bad33228)

Places