From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x736.google.com (mail-qk1-x736.google.com [IPv6:2607:f8b0:4864:20::736]) by sourceware.org (Postfix) with ESMTPS id 850903858D39 for ; Fri, 1 Jul 2022 08:40:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 850903858D39 Received: by mail-qk1-x736.google.com with SMTP id z12so1367870qki.3 for ; Fri, 01 Jul 2022 01:40:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ru3m+nH5wPektTAE6jbJOhlW9mxrF7x/xQWubdWrX+8=; b=RQrvMaQOUwv6+dAWKaLEZ5DmrwGff4faEhNwq2FNWaUbiS7PLwE1y+OPmNPyJHo6DD d2Kqmu/oqBmyAe9Mx9QEBspZoI4aylQzC4mjVafkJVPiTM8Jn32D3XshQnjPnYk0iAKy rn/bGacxT69+bD0d92EGCbPDF0NhvYq2MnK9OgXaLDQMeBT8gJg8qeaUlrzrDbQdUeSg uZzsmu3ZhHl+z3bzHqu+45PRPgY37AsYRzXfM24jmqypKRL9erOJPCturZ5nwtOgkpMj m8fShboyjGSJIgQX3b07h3hAyHh3MdMCg2lQcTDj8Du/puuVwZFF56sMFlZuQXPqQGMe djjw== X-Gm-Message-State: AJIora/FIOBnVg3InN4YyCK5njrF/tz62NtiFmZZjN9prXVryeGB0diN uMbpxhDth0XFUL6tvEDqdl+ph/Gl0yU+Afi5EYM= X-Google-Smtp-Source: AGRyM1u2BROq92vgr5njmmWj/lSlrXylYEYYvrjSJheUcwrqCbSwnmDz9Dzk70uTX+CR7yiyjEbQWonrKf8q7i7g1z0= X-Received: by 2002:a37:6793:0:b0:6ae:f690:1d9e with SMTP id b141-20020a376793000000b006aef6901d9emr9718670qkc.581.1656664814761; Fri, 01 Jul 2022 01:40:14 -0700 (PDT) MIME-Version: 1.0 References: <75d4b8f9-5ed2-de4c-d606-510a66714c9a@linux.ibm.com> <05835d48-50e9-be2c-2b60-1b42e9c6e5d6@linux.ibm.com> In-Reply-To: <05835d48-50e9-be2c-2b60-1b42e9c6e5d6@linux.ibm.com> From: Richard Biener Date: Fri, 1 Jul 2022 10:40:03 +0200 Message-ID: Subject: Re: PING^1 [PATCH] inline: Rebuild target option node for caller [PR105459] To: "Kewen.Lin" Cc: GCC Patches , Jakub Jelinek , Jan Hubicka , =?UTF-8?Q?Martin_Li=C5=A1ka?= Content-Type: multipart/mixed; boundary="000000000000cc1f3705e2ba551c" X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Jul 2022 08:40:18 -0000 --000000000000cc1f3705e2ba551c Content-Type: text/plain; charset="UTF-8" On Thu, Jun 23, 2022 at 4:03 AM Kewen.Lin wrote: > > Hi, > > Gentle ping https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596212.html > > BR, > Kewen > > on 2022/6/6 14:20, Kewen.Lin via Gcc-patches wrote: > > Hi, > > > > PR105459 exposes one issue in inline_call handling that when it > > decides to copy FP flags from callee to caller and rebuild the > > optimization node for caller fndecl, it's possible that the target > > option node is also necessary to be rebuilt. Without updating > > target option node early, it can make nodes share the same target > > option node wrongly, later when we want to unshare it somewhere > > (like in target hook) it can get unexpected results, like ICE on > > uninitialized secondary member of target globals exposed in this PR. I think that /* Reload global optimization flags. */ if (reload_optimization_node && DECL_STRUCT_FUNCTION (to->decl) == cfun) set_cfun (cfun, true); is supposed to do that via ix86_set_current_function which will eventually re-build the target optimization node exactly for this reason. But with LTO we arrive here during WPA time only and there cfun is NULL (and so is DECL_STRUCT_FUNCTION (to->decl)), so the target doesn't get the chance to fix things up here. Now, it should be fine to delay this fixup until we set the cfun at LTRANS time but there we run into if (old_tree != new_tree) { cl_target_option_restore (&global_options, &global_options_set, TREE_TARGET_OPTION (new_tree)); ... } else if (flag_unsafe_math_optimizations != TREE_TARGET_OPTION (new_tree)->x_ix86_unsafe_math_optimizations || (flag_excess_precision != TREE_TARGET_OPTION (new_tree)->x_ix86_excess_precision)) { ... FIXUP! ... and old_tree != new_tree disables the fixup. When we refactor the above to always consider the FP flag change (so apply it lazily), then this fixes the testcase in the PR as well. Thus something like the attached. Ideally this stuff would be refactored to a target hook that can work without the set_cfun, also working towards merging the target and optimization node since they have to be kept in sync ... I think your proposed patch makes another variant through the maze to do something at WPA time but that makes it all even more complicated :/ Sorry for the delay btw. Folks - any other opinions? Thanks, Richard. > > Commit r12-3721 makes it get exact fp_expression info and causes > > more optimization chances then exposes this issue. Commit r11-5855 > > introduces two target options to shadow flag_excess_precision and > > flag_unsafe_math_optimizations and shows the need to rebuild target > > node in inline_call when optimization node changes. > > > > As commented in PR105459, I tried to postpone init_function_start > > in cgraph_node::expand, but abandoned it since I thought it just > > concealed the issue. And I also tried to adjust the target node > > when current function switching, but failed since we get the NULL > > cfun and fndecl in WPA phase. > > > > Bootstrapped and regtested on x86_64-redhat-linux, powerpc64-linux-gnu > > P8 and powerpc64le-linux-gnu P9. > > > > Any thoughts? Is it OK for trunk? > > > > BR, > > Kewen > > ----- > > > > PR tree-optimization/105459 > > > > gcc/ChangeLog: > > > > * ipa-inline-transform.cc (inline_call): Rebuild target option node > > once optimization node gets rebuilt. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/lto/pr105459_0.c: New test. > > --- > > gcc/ipa-inline-transform.cc | 50 +++++++++++++++++++++++++-- > > gcc/testsuite/gcc.dg/lto/pr105459_0.c | 35 +++++++++++++++++++ > > 2 files changed, 83 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/lto/pr105459_0.c > > > > diff --git a/gcc/ipa-inline-transform.cc b/gcc/ipa-inline-transform.cc > > index 07288e57c73..edba58377f4 100644 > > --- a/gcc/ipa-inline-transform.cc > > +++ b/gcc/ipa-inline-transform.cc > > @@ -52,6 +52,7 @@ along with GCC; see the file COPYING3. If not see > > #include "ipa-modref.h" > > #include "symtab-thunks.h" > > #include "symtab-clones.h" > > +#include "target.h" > > > > int ncalls_inlined; > > int nfunctions_inlined; > > @@ -469,8 +470,53 @@ inline_call (struct cgraph_edge *e, bool update_original, > > } > > > > /* Reload global optimization flags. */ > > - if (reload_optimization_node && DECL_STRUCT_FUNCTION (to->decl) == cfun) > > - set_cfun (cfun, true); > > + if (reload_optimization_node) > > + { > > + /* Only need to check and update target option node > > + when target_option_default_node is not NULL. */ > > + if (target_option_default_node) > > + { > > + /* Save the current context for optimization and target option > > + node. */ > > + tree old_optimize > > + = build_optimization_node (&global_options, &global_options_set); > > + tree old_target_opt > > + = build_target_option_node (&global_options, &global_options_set); > > + > > + /* Restore optimization with new optimizatin node. */ > > + tree new_optimize = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (to->decl); > > + if (old_optimize != new_optimize) > > + cl_optimization_restore (&global_options, &global_options_set, > > + TREE_OPTIMIZATION (new_optimize)); > > + > > + /* Restore target option with the one from caller fndecl. */ > > + tree cur_target_opt = DECL_FUNCTION_SPECIFIC_TARGET (to->decl); > > + if (!cur_target_opt) > > + cur_target_opt = target_option_default_node; > > + cl_target_option_restore (&global_options, &global_options_set, > > + TREE_TARGET_OPTION (cur_target_opt)); > > + > > + /* Update target option as optimization changes. */ > > + targetm.target_option.override (); > > + > > + /* Rebuild target option node for caller fndecl and replace > > + with it if the node changes. */ > > + tree new_target_opt > > + = build_target_option_node (&global_options, &global_options_set); > > + if (cur_target_opt != new_target_opt) > > + DECL_FUNCTION_SPECIFIC_TARGET (to->decl) = new_target_opt; > > + > > + /* Restore the context with previous saved nodes. */ > > + if (old_optimize != new_optimize) > > + cl_optimization_restore (&global_options, &global_options_set, > > + TREE_OPTIMIZATION (old_optimize)); > > + cl_target_option_restore (&global_options, &global_options_set, > > + TREE_TARGET_OPTION (old_target_opt)); > > + } > > + > > + if (DECL_STRUCT_FUNCTION (to->decl) == cfun) > > + set_cfun (cfun, true); > > + } > > > > /* If aliases are involved, redirect edge to the actual destination and > > possibly remove the aliases. */ > > diff --git a/gcc/testsuite/gcc.dg/lto/pr105459_0.c b/gcc/testsuite/gcc.dg/lto/pr105459_0.c > > new file mode 100644 > > index 00000000000..c799e6ef23d > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/lto/pr105459_0.c > > @@ -0,0 +1,35 @@ > > +/* { dg-lto-do link } */ > > +/* { dg-lto-options { { -flto -O1 } } } */ > > + > > +double m; > > +int n; > > + > > +__attribute__ ((optimize ("-funsafe-math-optimizations"))) > > +void > > +bar (int x) > > +{ > > + n = x; > > + m = n; > > +} > > + > > +__attribute__ ((flatten)) > > +void > > +foo (int x) > > +{ > > + bar (x); > > +} > > + > > +void > > +quux (void) > > +{ > > + ++n; > > +} > > + > > +int > > +main (void) > > +{ > > + foo (0); > > + quux (); > > + > > + return 0; > > +} > > -- > > 2.27.0 --000000000000cc1f3705e2ba551c Content-Type: application/octet-stream; name=p Content-Disposition: attachment; filename=p Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_l527ccog0 ZGlmZiAtLWdpdCBhL2djYy9jb25maWcvaTM4Ni9pMzg2LW9wdGlvbnMuY2MgYi9nY2MvY29uZmln L2kzODYvaTM4Ni1vcHRpb25zLmNjCmluZGV4IGUxMWY2ODE4NmY1Li5hY2IyMjkxZTcwZiAxMDA2 NDQKLS0tIGEvZ2NjL2NvbmZpZy9pMzg2L2kzODYtb3B0aW9ucy5jYworKysgYi9nY2MvY29uZmln L2kzODYvaTM4Ni1vcHRpb25zLmNjCkBAIC0zMjMyLDI4ICszMjMyLDIyIEBAIGl4ODZfc2V0X2N1 cnJlbnRfZnVuY3Rpb24gKHRyZWUgZm5kZWNsKQogICBpZiAobmV3X3RyZWUgPT0gTlVMTF9UUkVF KQogICAgIG5ld190cmVlID0gdGFyZ2V0X29wdGlvbl9kZWZhdWx0X25vZGU7CiAKLSAgaWYgKG9s ZF90cmVlICE9IG5ld190cmVlKQorICBib29sIGZwX2ZsYWdfY2hhbmdlCisgICAgPSAoZmxhZ191 bnNhZmVfbWF0aF9vcHRpbWl6YXRpb25zCisgICAgICAgIT0gVFJFRV9UQVJHRVRfT1BUSU9OIChu ZXdfdHJlZSktPnhfaXg4Nl91bnNhZmVfbWF0aF9vcHRpbWl6YXRpb25zCisgICAgICAgfHwgKGZs YWdfZXhjZXNzX3ByZWNpc2lvbgorCSAgICE9IFRSRUVfVEFSR0VUX09QVElPTiAobmV3X3RyZWUp LT54X2l4ODZfZXhjZXNzX3ByZWNpc2lvbikpOworICBpZiAob2xkX3RyZWUgIT0gbmV3X3RyZWUg fHwgZnBfZmxhZ19jaGFuZ2UpCiAgICAgewogICAgICAgY2xfdGFyZ2V0X29wdGlvbl9yZXN0b3Jl ICgmZ2xvYmFsX29wdGlvbnMsICZnbG9iYWxfb3B0aW9uc19zZXQsCiAJCQkJVFJFRV9UQVJHRVRf T1BUSU9OIChuZXdfdHJlZSkpOwotICAgICAgaWYgKFRSRUVfVEFSR0VUX0dMT0JBTFMgKG5ld190 cmVlKSkKLQlyZXN0b3JlX3RhcmdldF9nbG9iYWxzIChUUkVFX1RBUkdFVF9HTE9CQUxTIChuZXdf dHJlZSkpOwotICAgICAgZWxzZSBpZiAobmV3X3RyZWUgPT0gdGFyZ2V0X29wdGlvbl9kZWZhdWx0 X25vZGUpCi0JcmVzdG9yZV90YXJnZXRfZ2xvYmFscyAoJmRlZmF1bHRfdGFyZ2V0X2dsb2JhbHMp OwotICAgICAgZWxzZQotCVRSRUVfVEFSR0VUX0dMT0JBTFMgKG5ld190cmVlKSA9IHNhdmVfdGFy Z2V0X2dsb2JhbHNfZGVmYXVsdF9vcHRzICgpOwotICAgIH0KLSAgZWxzZSBpZiAoZmxhZ191bnNh ZmVfbWF0aF9vcHRpbWl6YXRpb25zCi0JICAgIT0gVFJFRV9UQVJHRVRfT1BUSU9OIChuZXdfdHJl ZSktPnhfaXg4Nl91bnNhZmVfbWF0aF9vcHRpbWl6YXRpb25zCi0JICAgfHwgKGZsYWdfZXhjZXNz X3ByZWNpc2lvbgotCSAgICAgICAhPSBUUkVFX1RBUkdFVF9PUFRJT04gKG5ld190cmVlKS0+eF9p eDg2X2V4Y2Vzc19wcmVjaXNpb24pKQotICAgIHsKLSAgICAgIGNsX3RhcmdldF9vcHRpb25fcmVz dG9yZSAoJmdsb2JhbF9vcHRpb25zLCAmZ2xvYmFsX29wdGlvbnNfc2V0LAotCQkJCVRSRUVfVEFS R0VUX09QVElPTiAobmV3X3RyZWUpKTsKLSAgICAgIGl4ODZfZXhjZXNzX3ByZWNpc2lvbiA9IGZs YWdfZXhjZXNzX3ByZWNpc2lvbjsKLSAgICAgIGl4ODZfdW5zYWZlX21hdGhfb3B0aW1pemF0aW9u cyA9IGZsYWdfdW5zYWZlX21hdGhfb3B0aW1pemF0aW9uczsKLSAgICAgIERFQ0xfRlVOQ1RJT05f U1BFQ0lGSUNfVEFSR0VUIChmbmRlY2wpID0gbmV3X3RyZWUKLQk9IGJ1aWxkX3RhcmdldF9vcHRp b25fbm9kZSAoJmdsb2JhbF9vcHRpb25zLCAmZ2xvYmFsX29wdGlvbnNfc2V0KTsKKyAgICAgIGlm IChmcF9mbGFnX2NoYW5nZSkKKwl7CisJICBpeDg2X2V4Y2Vzc19wcmVjaXNpb24gPSBmbGFnX2V4 Y2Vzc19wcmVjaXNpb247CisJICBpeDg2X3Vuc2FmZV9tYXRoX29wdGltaXphdGlvbnMgPSBmbGFn X3Vuc2FmZV9tYXRoX29wdGltaXphdGlvbnM7CisJICBERUNMX0ZVTkNUSU9OX1NQRUNJRklDX1RB UkdFVCAoZm5kZWNsKSA9IG5ld190cmVlCisJICAgID0gYnVpbGRfdGFyZ2V0X29wdGlvbl9ub2Rl ICgmZ2xvYmFsX29wdGlvbnMsICZnbG9iYWxfb3B0aW9uc19zZXQpOworCX0KICAgICAgIGlmIChU UkVFX1RBUkdFVF9HTE9CQUxTIChuZXdfdHJlZSkpCiAJcmVzdG9yZV90YXJnZXRfZ2xvYmFscyAo VFJFRV9UQVJHRVRfR0xPQkFMUyAobmV3X3RyZWUpKTsKICAgICAgIGVsc2UgaWYgKG5ld190cmVl ID09IHRhcmdldF9vcHRpb25fZGVmYXVsdF9ub2RlKQo= --000000000000cc1f3705e2ba551c--