On Fri, Feb 22, 2019 at 10:57 AM Aldy Hernandez <al...@redhat.com> wrote:
>
> On 1/31/19 9:56 AM, Richard Biener wrote:
> > On Thu, Jan 31, 2019 at 3:45 PM Jakub Jelinek <ja...@redhat.com> wrote:
> >>
> >> On Thu, Jan 31, 2019 at 09:30:13AM -0500, Aldy Hernandez wrote:
> >>>>> (LOOPS_HAVE_PREHEADERS). This avoids unnecessary changes to the CFG.
> >>>>> But really, I'm just guessing here. LOOPS_NORMAL would also work,
> >>>>> albeit creating forwarder blocks.
> >>>>>
> >>>>> Tested on x86-64 Linux.
> >>>>>
> >>>>> What do you think?
> >>>>
> >>>> As far as I understand fold_return_value is true/false independent
> >>>> of -W* flags (but dependent on -fprintf-return-value or so). This means
> >>>> that your patch avoids the risk of CFG changes dependent on
> >>>> -W* flags. This means you could safely use LOOPS_NORMAL
> >>>
> >>> But isn't my code inside of ::execute, which is still gated by
> >>> fold_return_value AND all the -W* flags:?
> >>>
> >>> bool
> >>> pass_sprintf_length::gate (function *)
> >>> {
> >>> return ((warn_format_overflow > 0
> >>> || warn_format_trunc > 0
> >>> || flag_printf_return_value)
> >>> && (optimize > 0) == fold_return_value);
> >>> }
> >>>
> >>> Thus, I think we need to move the loop initialization somewhere else ??.
> >>
> >> Then perhaps turn the gate into just return (optimize > 0) ==
> >> fold_return_value;
> >> and in execute do what you're adding and guard the rest with the above
> >> condition? As -fprintf-return-value is on by default for C-family, it
> >> shouldn't
> >> change much.
> >> + adjust comment on the gate of course.
> >
> > Don't you alreday have
> >
> > @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
> > init_target_to_host_charmap ();
> >
> > calculate_dominance_info (CDI_DOMINATORS);
> > + bool optimizing_late = optimize > 0 && fold_return_value;
> > + if (optimizing_late)
> > + {
> > + /* ?? We should avoid changing the CFG as much as possible.
> > ...
> > + loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> > + scev_initialize ();
> > + }
> >
> > so loops are only initialized if fold_return_value is true? Ah - but that's
> > the pass parameter from params.def rather than the flag to enable
> > the folding... So indeed, change it to include && flag_printf_return_value
>
> I believe the flag_printf_return_value change was the final agreed upon
> solution.
>
> Tested on x86-64 Linux.
>
> OK for trunk?
OK.
Richard.
