| Message ID | Z+O0Z6uxVytPQWl5@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Ignore OPTION_MASK_SAVE_TOC_INDIRECT differences in inlining decisions [PR119327] | expand |
On 3/26/25 3:01 AM, Jakub Jelinek wrote: > In any case, this flag feels like a tuning decision rather than hard > ISA requirement and I see no problems why we couldn't inline > even explicit -msave-toc-indirect function into -mno-save-toc-indirect > or vice versa. > We already ignore OPTION_MASK_P{8,10}_FUSION which are also more > like tuning flags. I agree this is more a tuning decision and not an ISA requirement, so we should treat -m{no-,}save-toc-indirect similarly to the fusion options. LGTM, but I cannot approve it. Peter
On Wed, Mar 26, 2025 at 09:01:43AM +0100, Jakub Jelinek wrote: > The following testcase FAILs because the always_inline function can't > be inlined. > The rs6000 backend has similarly to other targets a hook which rejects > inlining which would bring in new ISAs which aren't there in the caller. > And this hook rejects this because of OPTION_MASK_SAVE_TOC_INDIRECT > differences. > This flag is set if explicitly requested or by default depending on > whether the current function looks hot (or at least not cold): > if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0 > && flag_shrink_wrap_separate > && optimize_function_for_speed_p (cfun)) > rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT; > The target nodes that are being compared here are actually the default > target node (which was created when cfun was NULL) vs. one that was > created for the always_inline function when it wasn't NULL, so one > doesn't have it, the other does. > In any case, this flag feels like a tuning decision rather than hard > ISA requirement and I see no problems why we couldn't inline > even explicit -msave-toc-indirect function into -mno-save-toc-indirect > or vice versa. > We already ignore OPTION_MASK_P{8,10}_FUSION which are also more > like tuning flags. It is okay for trunk. Thank you! (And thanks to Surya, for making it the focus of my attention)., > Bootstrapped/regtested on powerpc64le-linux, ok for trunk? > > 2025-03-26 Jakub Jelinek <jakub@redhat.com> > > PR target/119327 > * config/rs6000/rs6000.cc (rs6000_can_inline_p): Ignore also > OPTION_MASK_SAVE_TOC_INDIRECT differences. > > * g++.dg/opt/pr119327.C: New test. > > --- gcc/config/rs6000/rs6000.cc.jj 2025-03-18 14:56:37.990023768 +0100 > +++ gcc/config/rs6000/rs6000.cc 2025-03-25 13:21:33.174568536 +0100 > @@ -25765,10 +25765,13 @@ rs6000_can_inline_p (tree caller, tree c > } > } > > - /* Ignore -mpower8-fusion and -mpower10-fusion options for inlining > - purposes. */ > - callee_isa &= ~(OPTION_MASK_P8_FUSION | OPTION_MASK_P10_FUSION); > - explicit_isa &= ~(OPTION_MASK_P8_FUSION | OPTION_MASK_P10_FUSION); > + /* Ignore -mpower8-fusion, -mpower10-fusion and -msave-toc-indirect options > + for inlining purposes. */ > + HOST_WIDE_INT ignored_isas = (OPTION_MASK_P8_FUSION > + | OPTION_MASK_P10_FUSION The P8/P10 fusion parts are obviously correct. The "TOC fusion" part is harder, but that should never heve been a user option at all, it either is okay or it isn't. Maybe that thing should not be user-selectable at all. ("Maybe", like I guarantee I will okay a patch doing jsut that!) (Not "would", but "will", thanks) So yeah, tha patch is okay. Thank you! Segher
--- gcc/config/rs6000/rs6000.cc.jj 2025-03-18 14:56:37.990023768 +0100 +++ gcc/config/rs6000/rs6000.cc 2025-03-25 13:21:33.174568536 +0100 @@ -25765,10 +25765,13 @@ rs6000_can_inline_p (tree caller, tree c } } - /* Ignore -mpower8-fusion and -mpower10-fusion options for inlining - purposes. */ - callee_isa &= ~(OPTION_MASK_P8_FUSION | OPTION_MASK_P10_FUSION); - explicit_isa &= ~(OPTION_MASK_P8_FUSION | OPTION_MASK_P10_FUSION); + /* Ignore -mpower8-fusion, -mpower10-fusion and -msave-toc-indirect options + for inlining purposes. */ + HOST_WIDE_INT ignored_isas = (OPTION_MASK_P8_FUSION + | OPTION_MASK_P10_FUSION + | OPTION_MASK_SAVE_TOC_INDIRECT); + callee_isa &= ~ignored_isas; + explicit_isa &= ~ignored_isas; /* The callee's options must be a subset of the caller's options, i.e. a vsx function may inline an altivec function, but a no-vsx function --- gcc/testsuite/g++.dg/opt/pr119327.C.jj 2025-03-25 13:24:45.129988649 +0100 +++ gcc/testsuite/g++.dg/opt/pr119327.C 2025-03-25 13:25:09.513661266 +0100 @@ -0,0 +1,16 @@ +// PR target/119327 +// { dg-do compile { target c++11 } } +// { dg-options "-Os" } + +#pragma GCC optimize "fp-contract=off" + +template <class T> +void +foo (T f) +{ + f (); +} + +struct S { + S () { [] {}; foo ([] __attribute__((always_inline)) {}); } +} s;
Hi! The following testcase FAILs because the always_inline function can't be inlined. The rs6000 backend has similarly to other targets a hook which rejects inlining which would bring in new ISAs which aren't there in the caller. And this hook rejects this because of OPTION_MASK_SAVE_TOC_INDIRECT differences. This flag is set if explicitly requested or by default depending on whether the current function looks hot (or at least not cold): if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0 && flag_shrink_wrap_separate && optimize_function_for_speed_p (cfun)) rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT; The target nodes that are being compared here are actually the default target node (which was created when cfun was NULL) vs. one that was created for the always_inline function when it wasn't NULL, so one doesn't have it, the other does. In any case, this flag feels like a tuning decision rather than hard ISA requirement and I see no problems why we couldn't inline even explicit -msave-toc-indirect function into -mno-save-toc-indirect or vice versa. We already ignore OPTION_MASK_P{8,10}_FUSION which are also more like tuning flags. Bootstrapped/regtested on powerpc64le-linux, ok for trunk? 2025-03-26 Jakub Jelinek <jakub@redhat.com> PR target/119327 * config/rs6000/rs6000.cc (rs6000_can_inline_p): Ignore also OPTION_MASK_SAVE_TOC_INDIRECT differences. * g++.dg/opt/pr119327.C: New test. Jakub