Richard Biener <richard.guent...@gmail.com> writes:
> 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)
> ; ...
>
EnabledBy(funroll-loops || fpeel-loops) does not works as we expected:
"-funroll-loops -fno-peel-loops" turns off flag_cunroll_grow_size.
Through "EnabledBy", a flag can be turned, and also can be turned off by
the "EnabledBy option", only if the flag is not specifed through commond
line.
> and enable it at O3+. AUTODETECT_VALUE doesn't make sense for
> an option not supposed to be set by users?
>
global_options_set.x_flagxxx can be used to check if option is set by
user. But it does not work well here neither, because we also care of
if the flag is override by OPTION_OPTIMIZATION_TABLE or
OPTION_OVERRIDE.
AUTODETECT_VALUE(value is 2) is used for some flags like flag_web,
flag_rename_registers, flag_var_tracking, flag_tree_cselim...
And this way could be used to check if the flag is effective(on/off)
either explicit set by command line or implicit set through
OPTION_OVERRIDE or OPTION_OPTIMIZATION_TABLE.
So, I use it here.
Thanks again!
Jiufu
>
>> >
>> >> ; 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
>> >>
