On Thu, May 28, 2020 at 4:37 PM Jiufu Guo <guoji...@linux.ibm.com> wrote:
>
> Richard Biener <richard.guent...@gmail.com> writes:
>
> > On Thu, May 28, 2020 at 10:52 AM guojiufu <guoji...@linux.ibm.com> wrote:
> >>
> >> From: Jiufu Guo <guoji...@linux.ibm.com>
> >>
> >> Currently GIMPLE complete unroller(cunroll) is checking
> >> flag_unroll_loops and flag_peel_loops to see if allow size growth.
> >> Beside affects curnoll, flag_unroll_loops also controls RTL unroler.
> >> To have more freedom to control cunroll and RTL unroller, this patch
> >> introduces flag_cunroll_grow_size. With this patch, we can control
> >> cunroll and RTL unroller indepently.
> >>
> >> Bootstrap/regtest pass on powerpc64le. OK for trunk? And backport to
> >> GCC10 after week?
> >>
> >>
> >> +funroll-completely-grow-size
> >> +Var(flag_cunroll_grow_size) Init(2)
> >> +; Control cunroll to allow size growth during complete unrolling
> >> +
> >
> > So this really adds a new compiler option which would need
> > documenting.
> I once add 'Undocumented' (avoid shown in --help), and do not add
> 'Common' (avoid --help=common). What I want to do is avoid expose this
> to user.
> While this is still an option as you said.
>
> >
> > I fear we'll get into bikeshed territory here as well... I originally
> > thought
> > we can use
> >
> > Variable
> > int flag_cunroll_grow_size;
>
> Thanks, this code is definetly a variable instead an option. I would try
> this way.
> >
> > but now realize that does not work well with LTO without adjusting
> > the awk scripts to generate option saving/restoring. For your patch
> > you'd need to add 'Optimization' to get the flag streamed properly,
> > you should also verify the target adjustment done in the backend
> > is reflected in LTO mode.
>
> At here, internal option is relative simple 'Optimization' could help.
> When trying 'Variable', I will verify it in LTO mode.
It won't work without adjusting the awk scripts. So go with
funroll-completely-grow-size
Undocumented Optimization Var(flag_cunroll_grow_size)
EnabledBy(funroll-loops || fpeel-loops)
; ...
and enable it at O3+. AUTODETECT_VALUE doesn't make sense for
an option not supposed to be set by users?
> >
> >> ; Nonzero means that loop optimizer may assume that the induction
> >> variables
> >>
> >> + /* Allow cunroll to grow size accordingly. */
> >> + if (flag_cunroll_grow_size == AUTODETECT_VALUE)
> >> + flag_cunroll_grow_size = flag_unroll_loops || flag_peel_loops;
> >> +
> >
> > Any reason to not use EnabledBy(funroll-loops || fpeel-loops)?
>
> With tests and checking the generated code(e.g. options.c), I find that
> this setting has some unexpected behavior:
> For example, "-funroll-loops -fno-peel-loops" turns off the flag.
> "||" would indicate the flag will be _on/off_ by f[no]-unroll-loop or
> f[no]-peel-loops.
>
> >
> >> /* web and rename-registers help when run after loop unrolling. */
> >> if (flag_web == AUTODETECT_VALUE)
> >> flag_web = flag_unroll_loops;
>
> >> - unsigned int val = tree_unroll_loops_completely (flag_unroll_loops
> >> - || flag_peel_loops
> >> + unsigned int val = tree_unroll_loops_completely (flag_cunroll_grow_size
> >> || optimize >= 3, true);
> >
> > Given we check optimize >= 3 here please enable the flag by default
> > at O3+ via opts.c:default_options_table and also elide the optimize >= 3
> > check. That way -fno-unroll-completely-grow-size would have the desired
> > effect.
> >
> Actually in code flag_peel_loops is enabled at O3+, so, "|| optimize >=
> 3" could be removed. Like you said, this helps to set negative form
> even at -O3.
You are right.
> > Now back to the option name ... if we expose the option we should apply
> > some forward looking. Currently cunroll cannot be disabled or enabled
> > with a flag and the desired new flag simply tunes one knob on it. How
> > about adding
> >
> > -fcomplete-unroll-loops[=may-grow]
> -fcomplete-unroll-loops[=may-grow|inner|outer]
> >
> > to be able to further extend this later (there's the knob to only unroll
> > non-outermost loops and the knob whether to unroll loops where
> > intermediate exits are not statically predicted - incompletely controlled
> > by -fpeel-loops). There's unfortunately no existing examples that allows
> > multiple flags like -fcomlete-unroll-loops=may-grow,outer other than
> > the sanitizers which have manual option parsing.
> >
> > So if there's no good suggestion from option folks maybe go with
> >
> > -fcomplete-unroll-loops-may-grow
> >
> > (ick). And on a second thought -fcomplete-unroll-loops[=...] should
> > be -funroll-loops[={complete,may-grow,all}] to cover all unrolling
> > bases?
> >
> > I really hate to explode the number of options users have to
> > consider optimizing their code ...
> >
> > So if we can defer all this thinking and make a non-option flag
> > variable work that would be best IMHO.
> Yes, a few things are need trade-off when designing a user options.
>
>
> Jiufu
> >
> > Richard.
> >
> >> if (peeled_loops)
> >> {
> >> --
> >> 2.17.1
> >>
