yes, that is OK Thanks! Honza On Thu, Jan 8, 2026 at 8:01 AM Andrew Pinski <[email protected]> wrote:
> 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. > > > > > > > > > > >
