On Wed, Jan 7, 2026 at 10:41 PM Kugan Vivekanandarajah <[email protected]> wrote: > > Hi, > > > On 8 Jan 2026, at 5:37 am, Jan Hubička <[email protected]> wrote: > > > > External email: Use caution opening links or attachments > > Hello, > > sorry for being late here. > > The logic being doing fixup_cfg is the following. While passes like > > pure-const does change the function flags and technically breaks the CFG in > > all functions calling the function which has been discovered pure or const, > > they can not call fixup_cfg on all those callers since it would be > > quadratic and also the callers does not need to be load in memory. > > > > For this reason, we call fixup_cfg at the beginning of each block of local > > optimisations, where is a chance that pure/const or other IPA pass changed > > something relevant. > > This is the local optimisation passes (done in early opts) since first run > > of pure-const is there; then it is in the late optimisations for the same > > reason. Fixup after IPA passes happens in the inliner. > > > > ipa-pure-const/modref executed fixup cfg on function being compiled (in > > local mode) so later optimisations can continue. > > > > Now, what goes wrong here is that we added fnsplit as a subpass of autofdo. > > This is the same as tree-profile. Within tree-profile we fixup to fix > > precisely the same issue > > /* Local pure-const may imply need to fixup the cfg. */ > > if (gimple_has_body_p (node->decl) > > && (execute_fixup_cfg () & TODO_cleanup_cfg)) > > cleanup_tree_cfg (); > > So it would be more consistent if auto-profile did its own fixups too. (We > > will get more fixups with -fauto-profile, but less without, which is > > probably an overall win). > > Attached patch does it in similar way. > > Is this OK?
LGTM. > > Thanks, > Kugan > > > > > > > > Honza > > > > On Wed, Jan 7, 2026 at 1:18 AM Andrew Pinski > > <[email protected]> wrote: > > On Sun, Dec 21, 2025 at 9:56 PM Kugan Vivekanandarajah > > <[email protected]> wrote: > > > > > > Hi, > > > Thanks for the review. > > > > > > > On 21 Dec 2025, at 8:37 am, Andrew Pinski > > > > <[email protected]> wrote: > > > > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On Sun, Dec 14, 2025 at 1:18 PM Kugan Vivekanandarajah > > > > <[email protected]> wrote: > > > >> > > > >> Hi, > > > >> > > > >> This patch shows up with AutoFDO but IMO this is not limited to > > > >> AutoFDO. > > > >> > > > >> The bug is a stale Virtual SSA on calls to functions that have > > > >> been marked const or pure. > > > >> > > > >> pure_const pass analyzes function rocksdb::y::y() and determines it > > > >> has no side > > > >> effects and marks it as const. > > > >> > > > >> At this point, existing call sites to y::y() in other functions still > > > >> have: > > > >> # .MEM_12 = VDEF <.MEM_11> rocksdb::y::y (&l, _9); > > > >> > > > >> Exact ICE is: > > > >> cache/lru_cache.cc:921:1: error: virtual definition of statement not > > > >> up to date > > > >> # .MEM_12 = VDEF <.MEM_11> > > > >> rocksdb::y::y (&l, _9); > > > >> during GIMPLE pass: feedback_fnsplit > > > >> dump file: lru_cache_reduced.ii.076t.feedback_fnsplit1 > > > >> cache/lru_cache.cc:921:1: internal compiler error: verify_ssa failed > > > >> 0x26f4073 internal_error(char const*, ...) > > > >> ../../gcc-ro/gcc/diagnostic-global-context.cc:787 > > > >> 0x16f91df verify_ssa(bool, bool) > > > >> ../../gcc-ro/gcc/tree-ssa.cc:1203 > > > >> 0x12f0643 execute_function_todo > > > >> ../../gcc-ro/gcc/passes.cc:2104 > > > >> 0x12f0beb execute_todo > > > >> ../../gcc-ro/gcc/passes.cc:2149 > > > >> > > > >> > > > >> In ipa-pure-const, we do execute_fixup_cfg only for the changed. IMO, > > > >> we > > > >> should call for all the functions that has this function called. > > > >> Calling fixup for all also fixes this ICE. > > > >> > > > >> Later in feedback_fnsplit SSA verification fails. Added fixup > > > >> after execute_feedback_split_functions also fixes this ICE. > > > > This looks ok to me except I would add a comment on why you need the > > > > fixup and no reason for the todo variable either: > > > > > > > > /* Update virtual SSA as some functions could have changed to be > > > > pure/const. */ > > > > retval |= execute_fixup_cfg (); > > > > return retval; > > > > > > Added the comments. Is this OK now? > > > > Ok. (was waiting for the official announcement for being appointed as > > global reviewer before sending approval for this). > > > > Thanks, > > Andrew > > > > > > > > Thanks, > > > Kugan > > > > > > > > > > > > > > > > > Thanks, > > > > Andrew > > > > > > > >> > > > >> gcc/ChangeLog: > > > >> > > > >> 2025-12-08 Kugan Vivekanandarajah <[email protected]> > > > >> > > > >> * ipa-split.cc (execute_feedback_split_functions): Call > > > >> * execute_fixup_cfg. > > > > > > >
