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.
>
>> ; 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.
> 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
>>
