LP:602190(https://bugs.launchpad.net/gcc-linaro/+bug/602190) and LP:602285(https://bugs.launchpad.net/gcc-linaro/+bug/602285) are related to this patch below. You can get more details from comments of these bugs, since I've added my understand of the cause in comments.
This patch is to improve the performance of generated code, however, these two bugs are related to this patch(, correct me if I am wrong). Now, we have two options, 1) revert this patch, and make these test case pass; 2) keep this patch and fix test cases, 3) fix bugs and keep this patch,
What do you think?
2009-08-26 Daniel Jacobowitz dan@codesourcery.com
Issue #6131 - Enable additional optimizations by default in Lite Issue #6103 - Reduce default unrolling parameters at -O3
* release-notes-csl.xml (Improved optimization for ARM): New note.
gcc/ * config/arm/arm.c (arm_override_options): If flag_unroll_loops has the default value, adjust unrolling parameters. (arm_optimization_options): Set flag_unroll_loops to a default value. Enable flag_promote_loop_indices.
Modified: gcc/config/arm/arm.c ============================================================================== --- gcc/config/arm/arm.c (original) +++ gcc/config/arm/arm.c Fri Aug 28 14:41:19 2009 @@ -55,6 +55,7 @@ #include "langhooks.h" #include "df.h" #include "intl.h" +#include "params.h"
/* Forward definitions of types. */ typedef struct minipool_node Mnode; @@ -1857,6 +1858,29 @@ warning (0, "-low-irq-latency has no effect when compiling for the Thumb"); low_irq_latency = 0; + } + + /* CSL LOCAL */ + /* Loop unrolling can be a substantial win. At -O2, limit to 2x + unrolling by default to prevent excessive code growth; at -O3, + limit to 4x unrolling by default. We know we are not optimizing + for size if this is set (see arm_optimization_options). */ + if (flag_unroll_loops == 2) + { + if (optimize == 2) + { + flag_unroll_loops = 1; + if (!PARAM_SET_P (PARAM_MAX_UNROLL_TIMES)) + set_param_value ("max-unroll-times", 2); + } + else if (optimize > 2) + { + flag_unroll_loops = 1; + if (!PARAM_SET_P (PARAM_MAX_UNROLL_TIMES)) + set_param_value ("max-unroll-times", 4); + } + else + flag_unroll_loops = 0; } }
@@ -21175,6 +21199,17 @@ set_param_value ("max-inline-insns-single", 1); set_param_value ("max-inline-insns-auto", 1); } + else + { + /* CSL LOCAL */ + /* Set flag_unroll_loops to a default value, so that we can tell + if it was specified on the command line; see + arm_override_options. */ + flag_unroll_loops = 2; + /* Promote loop indices to int where possible. Consider moving this + to -Os, also. */ + flag_promote_loop_indices = 1; + } }
On Wed, Aug 04, 2010 at 10:53:46PM +0800, Yao Qi wrote:
LP:602190(https://bugs.launchpad.net/gcc-linaro/+bug/602190) and LP:602285(https://bugs.launchpad.net/gcc-linaro/+bug/602285) are related to this patch below. You can get more details from comments of these bugs, since I've added my understand of the cause in comments.
This patch is to improve the performance of generated code, however, these two bugs are related to this patch(, correct me if I am wrong). Now, we have two options, 1) revert this patch, and make these test case pass; 2) keep this patch and fix test cases, 3) fix bugs and keep this patch,
What do you think?
Julian, weren't you looking at these? You can avoid the param adjustments by an explicit -funroll-loops or -fno-unroll-loops, whichever is appropriate, in the options of the test case.
I do not think we should drop the patch.
On Wed, 4 Aug 2010 22:53:46 +0800 Yao Qi yao@codesourcery.com wrote:
LP:602190(https://bugs.launchpad.net/gcc-linaro/+bug/602190) and LP:602285(https://bugs.launchpad.net/gcc-linaro/+bug/602285) are related to this patch below. You can get more details from comments of these bugs, since I've added my understand of the cause in comments.
This patch is to improve the performance of generated code, however, these two bugs are related to this patch(, correct me if I am wrong). Now, we have two options, 1) revert this patch, and make these test case pass; 2) keep this patch and fix test cases, 3) fix bugs and keep this patch,
What do you think?
I don't particularly like the "CSL LOCAL" markers in a non-CSL-local branch, but I don't think that's what you're asking :-).
We're talking about the Linaro 4.4 branch here, right? I'm planning to add appropriate "-fno-unroll-loops" options to affected tests on the 4.5 branch (that is not done yet), but I wasn't planning to do the same for 4.4. I don't see why we can't though.
Julian
On Wed, Aug 04, 2010 at 04:42:49PM +0100, Julian Brown wrote:
On Wed, 4 Aug 2010 22:53:46 +0800 Yao Qi yao@codesourcery.com wrote:
LP:602190(https://bugs.launchpad.net/gcc-linaro/+bug/602190) and LP:602285(https://bugs.launchpad.net/gcc-linaro/+bug/602285) are related to this patch below. You can get more details from comments of these bugs, since I've added my understand of the cause in comments.
This patch is to improve the performance of generated code, however, these two bugs are related to this patch(, correct me if I am wrong). Now, we have two options, 1) revert this patch, and make these test case pass; 2) keep this patch and fix test cases, 3) fix bugs and keep this patch,
What do you think?
I don't particularly like the "CSL LOCAL" markers in a non-CSL-local branch, but I don't think that's what you're asking :-).
We're talking about the Linaro 4.4 branch here, right? I'm planning to add appropriate "-fno-unroll-loops" options to affected tests on the 4.5 branch (that is not done yet), but I wasn't planning to do the same for 4.4. I don't see why we can't though.
Yeah, I am talking about linaro 4.4.
"-fno-unroll-loops" is useful here, however it is a little bit like "distant water cannot quench present thirst" (a Chinese idiom). Maybe, we can fix them on 4.5 once your -fno-unroll-loops is ready.
Micheal/Ulrich, your comment is welcome.
Send it again to linaro-toolchain@lists.linaro.org
On Wed, Aug 04, 2010 at 10:53:46PM +0800, Yao Qi wrote:
LP:602190(https://bugs.launchpad.net/gcc-linaro/+bug/602190) and LP:602285(https://bugs.launchpad.net/gcc-linaro/+bug/602285) are related to this patch below. You can get more details from comments of these bugs, since I've added my understand of the cause in comments.
This patch is to improve the performance of generated code, however, these two bugs are related to this patch(, correct me if I am wrong). Now, we have two options, 1) revert this patch, and make these test case pass; 2) keep this patch and fix test cases, 3) fix bugs and keep this patch,
What do you think?
2009-08-26 Daniel Jacobowitz dan@codesourcery.com
Issue #6131 - Enable additional optimizations by default in Lite Issue #6103 - Reduce default unrolling parameters at -O3
- release-notes-csl.xml (Improved optimization for ARM): New note.
gcc/
- config/arm/arm.c (arm_override_options): If flag_unroll_loops has
the default value, adjust unrolling parameters. (arm_optimization_options): Set flag_unroll_loops to a default value. Enable flag_promote_loop_indices.
Modified: gcc/config/arm/arm.c
--- gcc/config/arm/arm.c (original) +++ gcc/config/arm/arm.c Fri Aug 28 14:41:19 2009 @@ -55,6 +55,7 @@ #include "langhooks.h" #include "df.h" #include "intl.h" +#include "params.h" /* Forward definitions of types. */ typedef struct minipool_node Mnode; @@ -1857,6 +1858,29 @@ warning (0, "-low-irq-latency has no effect when compiling for the Thumb"); low_irq_latency = 0;
- }
- /* CSL LOCAL */
- /* Loop unrolling can be a substantial win. At -O2, limit to 2x
unrolling by default to prevent excessive code growth; at -O3,
limit to 4x unrolling by default. We know we are not optimizing
for size if this is set (see arm_optimization_options). */
- if (flag_unroll_loops == 2)
- {
if (optimize == 2)
- {
flag_unroll_loops = 1;
if (!PARAM_SET_P (PARAM_MAX_UNROLL_TIMES))
set_param_value ("max-unroll-times", 2);
- }
else if (optimize > 2)
- {
flag_unroll_loops = 1;
if (!PARAM_SET_P (PARAM_MAX_UNROLL_TIMES))
set_param_value ("max-unroll-times", 4);
- }
else
- flag_unroll_loops = 0; }
} @@ -21175,6 +21199,17 @@ set_param_value ("max-inline-insns-single", 1); set_param_value ("max-inline-insns-auto", 1); }
- else
- {
/* CSL LOCAL */
/* Set flag_unroll_loops to a default value, so that we can tell
if it was specified on the command line; see
arm_override_options. */
flag_unroll_loops = 2;
/* Promote loop indices to int where possible. Consider moving this
to -Os, also. */
flag_promote_loop_indices = 1;
- }
}
-- Yao Qi CodeSourcery yao@codesourcery.com (650) 331-3385 x739
linaro-toolchain@lists.linaro.org