Re: [PATCH] LoongArch: Set 4 * (issue rate) as the default for -falign-functions and -falign-loops

[WANG Xuerui <i@xen0n.name>]

On 2023/4/18 20:45, Xi Ruoyao wrote:
> On Tue, 2023-04-18 at 20:39 +0800, WANG Xuerui wrote:
>> Hi,
>>
>> Thanks for helping confirming on GCC and porting this! I'd never know
>> even GCC lacked this adaptation without someone actually checking... Too
>> many things are taken for granted these days.
>>
>> On 2023/4/18 20:17, Xi Ruoyao wrote:
>>> According to Xuerui's LLVM changeset [1], doing so can make a
>>> significant performace gain.

"doing so can gain significant performance" or "significant performance 
can be gained by ..."?

Also the other important thing is, guaranteeing alignment also makes 
performance *more predictable* in addition to generally making things 
faster. You may want to mention this too somewhere.

>>>
>>> Bootstrapped and regtested on loongarch64-linux-gnu.  Ok for GCC 14?
>>>
>>> [1]:https://reviews.llvm.org/D148622
nit: one space after the colon.
>>>
>>> gcc/ChangeLog:
>>>
>>>          * config/loongarch/loongarch.cc
>>>          (loongarch_option_override_internal): If -falign-functions is
>>>          used but the alignment is not explicitly specified, set it to
>>>          4 * loongarch_issue_rate ().  Likewise for -falign-loops.
>>> ---
>>>    gcc/config/loongarch/loongarch.cc | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
>>> index 06fc1cd0604..6552484de7c 100644
>>> --- a/gcc/config/loongarch/loongarch.cc
>>> +++ b/gcc/config/loongarch/loongarch.cc
>>> @@ -6236,6 +6236,17 @@ loongarch_option_override_internal (struct gcc_options *opts)
>>>          && !opts->x_optimize_size)
>>>        opts->x_flag_prefetch_loop_arrays = 1;
>>>    
>>> +  /* Align functions and loops to (issue rate) * (insn size) to improve
>>> +     the throughput of the fetching units.  */
>> What about gating all of these on !opts->x_optimize_size, similar to
>> what aarch64 does?
> opts->x_flag_align_functions and opts->x_flag_align_loops are only set
> with -O2 or above unless the user manually uses -falign-functions or -
> falign-loops.  If the user uses "-Os -falign-functions" as CFLAGS I'd
> assume s(he) wants to optimize for size but keep the optimized function
> alignment.

Ah, okay. Fine then.

BTW I've also added some comments to the commit message that I forgot to 
review earlier.

>>> +  char *align = XNEWVEC (char, 16);
>>> +  sprintf (align, "%d", loongarch_issue_rate () * 4);
>>> +
>>> +  if (opts->x_flag_align_functions && !opts->x_str_align_functions)
>>> +    opts->x_str_align_functions = align;
>>> +
>>> +  if (opts->x_flag_align_loops && !opts->x_str_align_loops)
>>> +    opts->x_str_align_loops = align;
>>> +
>>>      if (TARGET_DIRECT_EXTERN_ACCESS && flag_shlib)
>>>        error ("%qs cannot be used for compiling a shared library",
>>>             "-mdirect-extern-access");
>> Otherwise LGTM, thanks!