Re: [PATCH][ARM] Improve max_insns_skipped logic

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

On 27/06/17 16:38, Wilco Dijkstra wrote:
>
>
> ping
>
>
> From: Wilco Dijkstra
> Sent: 10 November 2016 17:19
> To: GCC Patches
> Cc: nd
> Subject: [PATCH][ARM] Improve max_insns_skipped logic
>
> Improve the logic when setting max_insns_skipped.  Limit the maximum 
> size of IT
> to MAX_INSN_PER_IT_BLOCK as otherwise multiple IT instructions are needed,
> increasing codesize.  Given 4 works well for Thumb-2, use the same 
> limit for ARM
> for consistency.
>
> ChangeLog:
> 2016-11-04  Wilco Dijkstra  <wdijkstr@arm.com>
>
>         * config/arm/arm.c (arm_option_params_internal): Improve 
> setting of
>         max_insns_skipped.
> --
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 
> f046854e9665d54911616fc1c60fee407188f7d6..29e8d1d07d918fbb2a627a653510dfc8587ee01a 
> 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -2901,20 +2901,12 @@ arm_option_params_internal (void)
>        targetm.max_anchor_offset = TARGET_MAX_ANCHOR_OFFSET;
>      }
>
> -  if (optimize_size)
> -    {
> -      /* If optimizing for size, bump the number of instructions that we
> -         are prepared to conditionally execute (even on a StrongARM).  */
> -      max_insns_skipped = 6;
> +  /* Increase the number of conditional instructions with -Os.  */
> +  max_insns_skipped = optimize_size ? 4 : 
> current_tune->max_insns_skipped;
>
> -      /* For THUMB2, we limit the conditional sequence to one IT 
> block.  */
> -      if (TARGET_THUMB2)
> -        max_insns_skipped = arm_restrict_it ? 1 : 4;
> -    }
> -  else
> -    /* When -mrestrict-it is in use tone down the if-conversion.  */
> -    max_insns_skipped = (TARGET_THUMB2 && arm_restrict_it)
> -      ? 1 : current_tune->max_insns_skipped;
> +  /* For THUMB2, we limit the conditional sequence to one IT block.  */
> +  if (TARGET_THUMB2)
> +    max_insns_skipped = MIN (max_insns_skipped, MAX_INSN_PER_IT_BLOCK);

I like the simplifications in the selection logic here :)
However, changing the value for ARM from 6 to 4 looks a bit arbitrary to me.
There's probably a reason why default values for ARM and Thumb-2 are 
different
(maybe not a good one) and I'd rather not change it without some code 
size data measurements.
So I'd rather not let that hold this cleanup patch though, so this is ok
  (assuming a normal bootstrap and testing cycle) without changing the 6 
to a 4
and you can propose a change to 4 as a separate patch that can be 
discussed on its own.

Thanks,
Kyrill

>  }
>
>  /* True if -mflip-thumb should next add an attribute for the default
>