public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][ARM] Improve max_insns_skipped logic
@ 2016-11-10 17:19 Wilco Dijkstra
  2016-11-11 10:32 ` Richard Earnshaw
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2016-11-10 17:19 UTC (permalink / raw)
  To: GCC Patches; +Cc: nd

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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-11-10 17:19 [PATCH][ARM] Improve max_insns_skipped logic Wilco Dijkstra
@ 2016-11-11 10:32 ` Richard Earnshaw
  2016-11-11 11:42   ` Wilco Dijkstra
  2016-12-06 15:05 ` Wilco Dijkstra
  2017-04-20 16:05 ` Wilco Dijkstra
  2 siblings, 1 reply; 15+ messages in thread
From: Richard Earnshaw @ 2016-11-11 10:32 UTC (permalink / raw)
  To: Wilco Dijkstra, GCC Patches; +Cc: nd

On 10/11/16 17:19, Wilco Dijkstra wrote:
> 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.  
> 

You don't provide any information about what benefits this brings.

> Given 4 works well for Thumb-2, use the same limit for ARM
> for consistency.

Why?  Logic might suggest that given thumb has to execute an IT
instruction first, then allowing ARM to have one more conditional
instruction is the best answer.

R.

> 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);
>  }
>  
>  /* True if -mflip-thumb should next add an attribute for the default
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-11-11 10:32 ` Richard Earnshaw
@ 2016-11-11 11:42   ` Wilco Dijkstra
  2016-11-14 14:15     ` Wilco Dijkstra
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2016-11-11 11:42 UTC (permalink / raw)
  To: Richard Earnshaw, GCC Patches; +Cc: nd

Richard Earnshaw wrote:
> On 10/11/16 17:19, Wilco Dijkstra wrote:
> > 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.  
>
> You don't provide any information about what benefits this brings.

It reduces codesize and improves performance as you avoid emitting a
second IT instruction for sequences of more than 4 conditional instructions.

> > Given 4 works well for Thumb-2, use the same limit for ARM
> > for consistency.
>
> Why?  Logic might suggest that given thumb has to execute an IT
> instruction first, then allowing ARM to have one more conditional
> instruction is the best answer.

Long conditional sequences are slow on modern cores - the value 6 for
max_insns_skipped is a few decades out of date as it was meant for ARM2!
Even with -Os the performance loss for larger values is not worth the
small codesize gain (there are many better options to reduce codesize
that actually improve performance at the same time). So using the same
code generation heuristics for ARM and Thumb-2 is a good idea.

Wilco

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-11-11 11:42   ` Wilco Dijkstra
@ 2016-11-14 14:15     ` Wilco Dijkstra
  0 siblings, 0 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2016-11-14 14:15 UTC (permalink / raw)
  To: Richard Earnshaw, GCC Patches; +Cc: nd

Wilco Dijkstra wrote:
> Richard Earnshaw wrote:
> On 10/11/16 17:19, Wilco Dijkstra wrote:

> Long conditional sequences are slow on modern cores - the value 6 for
> max_insns_skipped is a few decades out of date as it was meant for ARM2!
> Even with -Os the performance loss for larger values is not worth the
> small codesize gain (there are many better options to reduce codesize
> that actually improve performance at the same time). So using the same
> code generation heuristics for ARM and Thumb-2 is a good idea.

A simple codesize comparison on CSiBE shows using 4 rather than 6 for
max_insns_skipped is just 0.07% larger on ARM with -Os. So it's not
obvious that increasing max_insns_skipped in -Os is a useful codesize
optimization...

Wilco
    

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-11-10 17:19 [PATCH][ARM] Improve max_insns_skipped logic Wilco Dijkstra
  2016-11-11 10:32 ` Richard Earnshaw
@ 2016-12-06 15:05 ` Wilco Dijkstra
  2016-12-14 16:39   ` Wilco Dijkstra
  2017-04-20 16:05 ` Wilco Dijkstra
  2 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2016-12-06 15:05 UTC (permalink / raw)
  To: GCC Patches, Richard Earnshaw; +Cc: nd


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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

    

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-12-06 15:05 ` Wilco Dijkstra
@ 2016-12-14 16:39   ` Wilco Dijkstra
  2017-01-17 12:16     ` Wilco Dijkstra
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2016-12-14 16:39 UTC (permalink / raw)
  To: GCC Patches, Richard Earnshaw; +Cc: nd


    

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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

        

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-12-14 16:39   ` Wilco Dijkstra
@ 2017-01-17 12:16     ` Wilco Dijkstra
  2017-02-02 14:45       ` Wilco Dijkstra
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2017-01-17 12:16 UTC (permalink / raw)
  To: GCC Patches, Richard Earnshaw; +Cc: nd


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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

            

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-01-17 12:16     ` Wilco Dijkstra
@ 2017-02-02 14:45       ` Wilco Dijkstra
  2017-02-23 16:58         ` Wilco Dijkstra
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2017-02-02 14:45 UTC (permalink / raw)
  To: GCC Patches, Richard Earnshaw, Kyrylo Tkachov; +Cc: nd


    

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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

                

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-02-02 14:45       ` Wilco Dijkstra
@ 2017-02-23 16:58         ` Wilco Dijkstra
  0 siblings, 0 replies; 15+ messages in thread
From: Wilco Dijkstra @ 2017-02-23 16:58 UTC (permalink / raw)
  To: GCC Patches, Richard Earnshaw, Kyrylo Tkachov; +Cc: nd


    

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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

                    

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2016-11-10 17:19 [PATCH][ARM] Improve max_insns_skipped logic Wilco Dijkstra
  2016-11-11 10:32 ` Richard Earnshaw
  2016-12-06 15:05 ` Wilco Dijkstra
@ 2017-04-20 16:05 ` Wilco Dijkstra
  2017-06-13 14:00   ` Wilco Dijkstra
  2 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2017-04-20 16:05 UTC (permalink / raw)
  To: GCC Patches, Kyrylo Tkachov; +Cc: nd, Richard Earnshaw


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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

    

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-04-20 16:05 ` Wilco Dijkstra
@ 2017-06-13 14:00   ` Wilco Dijkstra
  2017-06-27 15:38     ` Wilco Dijkstra
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2017-06-13 14:00 UTC (permalink / raw)
  To: GCC Patches, Kyrylo Tkachov; +Cc: nd, Richard Earnshaw


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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

        

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-06-13 14:00   ` Wilco Dijkstra
@ 2017-06-27 15:38     ` Wilco Dijkstra
  2017-09-04 16:52       ` Kyrill Tkachov
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2017-06-27 15:38 UTC (permalink / raw)
  To: GCC Patches, Kyrylo Tkachov; +Cc: nd, Richard Earnshaw

    

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);
 }
 
 /* True if -mflip-thumb should next add an attribute for the default

            

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-06-27 15:38     ` Wilco Dijkstra
@ 2017-09-04 16:52       ` Kyrill Tkachov
  2017-09-05 10:32         ` Wilco Dijkstra
  0 siblings, 1 reply; 15+ messages in thread
From: Kyrill Tkachov @ 2017-09-04 16:52 UTC (permalink / raw)
  To: Wilco Dijkstra, GCC Patches; +Cc: nd, Richard Earnshaw


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
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-09-04 16:52       ` Kyrill Tkachov
@ 2017-09-05 10:32         ` Wilco Dijkstra
  2017-09-05 17:16           ` Kyrill Tkachov
  0 siblings, 1 reply; 15+ messages in thread
From: Wilco Dijkstra @ 2017-09-05 10:32 UTC (permalink / raw)
  To: Kyrill Tkachov, GCC Patches; +Cc: nd, Richard Earnshaw

Kyrill Tkachov wrote:

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

To quote myself from the thread:

Long conditional sequences are slow on modern cores - the value 6 for
max_insns_skipped is a few decades out of date as it was meant for ARM2!
Even with -Os the performance loss for larger values is not worth the
small codesize gain (there are many better options to reduce codesize
that actually improve performance at the same time). So using the same
code generation heuristics for ARM and Thumb-2 is a good idea.

A simple codesize comparison on CSiBE shows using 4 rather than 6 for
max_insns_skipped is just 0.07% larger on ARM with -Os. So it's not
obvious that increasing max_insns_skipped in -Os is a useful codesize
optimization...

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

Based on the above is that really needed? What specific problem do you
expect to occur with the value 4? 

Wilco

    

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH][ARM] Improve max_insns_skipped logic
  2017-09-05 10:32         ` Wilco Dijkstra
@ 2017-09-05 17:16           ` Kyrill Tkachov
  0 siblings, 0 replies; 15+ messages in thread
From: Kyrill Tkachov @ 2017-09-05 17:16 UTC (permalink / raw)
  To: Wilco Dijkstra, GCC Patches; +Cc: nd, Richard Earnshaw


On 05/09/17 11:32, Wilco Dijkstra wrote:
> Kyrill Tkachov wrote:
>
>> 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.
> To quote myself from the thread:
>
> Long conditional sequences are slow on modern cores - the value 6 for
> max_insns_skipped is a few decades out of date as it was meant for ARM2!
> Even with -Os the performance loss for larger values is not worth the
> small codesize gain (there are many better options to reduce codesize
> that actually improve performance at the same time). So using the same
> code generation heuristics for ARM and Thumb-2 is a good idea.
>
> A simple codesize comparison on CSiBE shows using 4 rather than 6 for
> max_insns_skipped is just 0.07% larger on ARM with -Os. So it's not
> obvious that increasing max_insns_skipped in -Os is a useful codesize
> optimization...
>
>> 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.
> Based on the above is that really needed? What specific problem do you
> expect to occur with the value 4?

Nothing in particular, thanks for providing some numbers.
I think unifying the heuristics for ARM and Thumb2 makes sense and a 
0.07% code size
hit on ARM (i.e. not code-size-optimised Thumb-2) mode seems acceptable 
to me if it allows more performant code.

So this is ok then.
Thanks,
Kyrill

> Wilco
>
>      

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-09-05 17:16 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-10 17:19 [PATCH][ARM] Improve max_insns_skipped logic Wilco Dijkstra
2016-11-11 10:32 ` Richard Earnshaw
2016-11-11 11:42   ` Wilco Dijkstra
2016-11-14 14:15     ` Wilco Dijkstra
2016-12-06 15:05 ` Wilco Dijkstra
2016-12-14 16:39   ` Wilco Dijkstra
2017-01-17 12:16     ` Wilco Dijkstra
2017-02-02 14:45       ` Wilco Dijkstra
2017-02-23 16:58         ` Wilco Dijkstra
2017-04-20 16:05 ` Wilco Dijkstra
2017-06-13 14:00   ` Wilco Dijkstra
2017-06-27 15:38     ` Wilco Dijkstra
2017-09-04 16:52       ` Kyrill Tkachov
2017-09-05 10:32         ` Wilco Dijkstra
2017-09-05 17:16           ` Kyrill Tkachov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).