Re: [PATCH 3/3] Extend -falign-FOO=N to N[:M[:N2[:M2]]]

[Martin Liška <mliska at suse dot cz>]

On 07/03/2018 11:55 AM, Segher Boessenkool wrote:
> On Tue, Jul 03, 2018 at 10:53:20AM +0200, Martin Liška wrote:
>> On 06/29/2018 09:04 PM, Jeff Law wrote:
>>> I think this is fine for the trunk.
>>>
>>> jeff
>>
>> Thank you Jeff.
>>
>> I found some issues when doing build of all targets (contrib/config-list.mk).
>> I'll update patch and test that affected cross-compilers still produce same output.
>>
>> However I noticed one ppc64 issue:
>>
>> $ cat -n gcc/config/powerpcspe/powerpcspe.c
>>
>>   5401        /* Set branch target alignment, if not optimizing for size.  */
>>   5402        if (!optimize_size)
>>   5403          {
>>   5404            /* Cell wants to be aligned 8byte for dual issue.  Titan wants to be
>>   5405               aligned 8byte to avoid misprediction by the branch predictor.  */
>>   5406            if (rs6000_cpu == PROCESSOR_TITAN
>>   5407                || rs6000_cpu == PROCESSOR_CELL)
>>   5408              {
>>   5409                if (align_functions <= 0)
>>   5410                  align_functions = 8;
>>   5411                if (align_jumps <= 0)
>>   5412                  align_jumps = 8;
>>   5413                if (align_loops <= 0)
>>   5414                  align_loops = 8;
>>   5415              }
>>   5416            if (rs6000_align_branch_targets)
>>   5417              {
>>   5418                if (align_functions <= 0)
>>   5419                  align_functions = 16;
>>   5420                if (align_jumps <= 0)
>>   5421                  align_jumps = 16;
>>   5422                if (align_loops <= 0)
>>   5423                  {
>>   5424                    can_override_loop_align = 1;
>>   5425                    align_loops = 16;
>>   5426                  }
>>   5427              }
>>   5428            if (align_jumps_max_skip <= 0)
>>   5429              align_jumps_max_skip = 15;
>>   5430            if (align_loops_max_skip <= 0)
>>   5431              align_loops_max_skip = 15;
>>
>> Note that at line 5429 there's set of align_jumps_max_skip to 15 if not set by default.
>> At line 5412 align_jumps is set to 8, and align_jumps_max_skip should be equal align_jumps - 1.
>> That's a discrepancy. Segher can you please take a look?
> 
> This is powerpcspe, that's not mine.
> 
> But rs6000 has the same code, sure.

Right, that why I wrote to you.

> Why do you say "align_jumps_max_skip
> should be equal align_jumps - 1"?  If that were true, why does it exist
> at all?
> 
> toplev.c already has (in init_alignments):
> 
>   if (align_jumps_max_skip > align_jumps)
>     align_jumps_max_skip = align_jumps - 1;

I'm rewriting this logic in the patch set. Issue is that 
checking for value of align_jumps_max_skip is done
in rs6000_option_override_internal, which is place before
align_jumps_max_skip is parsed.

That said, 'align_jumps_max_skip <= 0' is always true.

Martin

> 
> so why would targets duplicate that logic?  (The target override is called
> before init_alignments).
> 
> 
> Segher
>