| Message ID | 4949065.31r3eYUQgx@fomalhaut |
|---|---|
| State | New |
| Headers | show |
| Series | Fix wrong code out of NRV + RSO + inlining | expand |
On Wed, Sep 11, 2024 at 10:26 AM Eric Botcazou <botcazou@adacore.com> wrote: > > Hi, > > the attached Ada testcase compiled with -O -flto exhibits a wrong code issue > when the 3 optimizations NRV + RSO + inlining are applied to the same call: if > the LHS of the call is marked write-only before inlining, then it will keep > the mark after inlining although it may be read in GIMPLE from that point on. > > The proposed fix is to always clear the flag during inlining in the RSO case. > Tested on x86-64/Linux, OK for the mainline? Hmm, it looks to me that the IPA analysis marking the variable readonly in the first place is wrong - or that NRV may not be applied to such a variable later. Is NRV ever applied to say static const S s = ...; s = foo (); thus a readonly declared LHS? I think adjusting the varpool flag just during inline materialization doesn't resolve issues that appear when the wrong flag caused other wrong IPA decisions for example. Richard. > > 2024-09-11 Eric Botcazou <ebotcazou@adacore.com> > > * tree-inline.cc (declare_return_variable): Clear writeonly flag on > a global variable used directly as the return slot. > > 2024-09-11 Eric Botcazou <ebotcazou@adacore.com> > > * gnat.dg/lto27.adb: New test. > * gnat.dg/lto27_pkg1.ads, gnat.dg/lto27_pkg2.ads, > gnat.dg/lto27_pkg2.adb, gnat.dg/lto27_pkg3.ads: New helper. > > > -- > Eric Botcazou
> Hmm, it looks to me that the IPA analysis marking the variable readonly > in the first place is wrong - or that NRV may not be applied to such a > variable later. Is NRV ever applied to say > > static const S s = ...; > > s = foo (); > > thus a readonly declared LHS? But NRV is only an example and not necessary, as you may read a RESULT_DECL in the callee. So the combination is actually just RSO + inlining if the callee happens to read RESULT_DECL.
On Wed, Sep 11, 2024 at 12:38 PM Eric Botcazou <botcazou@adacore.com> wrote: > > > Hmm, it looks to me that the IPA analysis marking the variable readonly > > in the first place is wrong - or that NRV may not be applied to such a > > variable later. Is NRV ever applied to say > > > > static const S s = ...; > > > > s = foo (); > > > > thus a readonly declared LHS? > > But NRV is only an example and not necessary, as you may read a RESULT_DECL in > the callee. So the combination is actually just RSO + inlining if the callee > happens to read RESULT_DECL. Sure, but then the same argument is that with RSO marking the variable as write-only is wrong (or changing the call to use RSO for a write-only variable) - unless the callee never reads from RESULT_DECL. Richard. > > -- > Eric Botcazou > >
diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc index f31a34ac410..2bb1e1602b2 100644 --- a/gcc/tree-inline.cc +++ b/gcc/tree-inline.cc @@ -3782,6 +3782,11 @@ declare_return_variable (copy_body_data *id, tree return_slot, tree modify_dest, gcc_assert (TREE_CODE (var) != SSA_NAME); if (TREE_ADDRESSABLE (result)) mark_addressable (var); + /* RESULT may also be read in the callee, typically because the NRV + optimization has been applied to the function, so VAR may also be + read from now on. */ + if (VAR_P (var) && (TREE_STATIC (var) || DECL_EXTERNAL (var))) + varpool_node::get (var)->writeonly = 0; } if (DECL_NOT_GIMPLE_REG_P (result) && DECL_P (var))