[PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Carrot Wei]

This patch fixes
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41705

For target ARM HAVE_conditional_execution is enabled because it is supported
by ARM ISA. But when compiled to thumb1 instructions, conditional execution is
not supported, the enabled HAVE_conditional_execution will disable some if
conversion optimizations.

Change the macro definition to (!TARGET_THUMB1) so compiler can get the correct
value according to the target ISA.

Test:
This patch was applied to trunk GCC and tested on the arm emulator with newlib.
No new failure.


ChangeLog:
2009-10-16  Wei Guozhi  <carrot@google.com>

        PR target/41705
        * config/arm/arm.h : New definition for macro
        HAVE_conditional_execution.
        * ifcvt.c : Change the header files order.

thanks
Guozhi



Index: arm.h
===================================================================
--- arm.h       (revision 152888)
+++ arm.h       (working copy)
@@ -2753,4 +2753,10 @@ enum arm_builtins
 #define NEED_INDICATE_EXEC_STACK       0
 #endif

+/* TARGET_THUMB1 doesn't have conditional execution capability.  */
+#ifdef HAVE_conditional_execution
+#undef HAVE_conditional_execution
+#endif
+#define HAVE_conditional_execution (!TARGET_THUMB1)
+
 #endif /* ! GCC_ARM_H */


Index: ifcvt.c
===================================================================
--- ifcvt.c     (revision 152888)
+++ ifcvt.c     (working copy)
@@ -21,13 +21,13 @@
 #include "config.h"
 #include "system.h"
 #include "coretypes.h"
+#include "insn-config.h"
 #include "tm.h"

 #include "rtl.h"
 #include "regs.h"
 #include "function.h"
 #include "flags.h"
-#include "insn-config.h"
 #include "recog.h"
 #include "except.h"
 #include "hard-reg-set.h"

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Ian Lance Taylor]

Carrot Wei <carrot@google.com> writes:

> Index: ifcvt.c
> ===================================================================
> --- ifcvt.c     (revision 152888)
> +++ ifcvt.c     (working copy)
> @@ -21,13 +21,13 @@
>  #include "config.h"
>  #include "system.h"
>  #include "coretypes.h"
> +#include "insn-config.h"
>  #include "tm.h"
>
>  #include "rtl.h"
>  #include "regs.h"
>  #include "function.h"
>  #include "flags.h"
> -#include "insn-config.h"
>  #include "recog.h"
>  #include "except.h"
>  #include "hard-reg-set.h"


No.  The order of the main #includes is fairly canonical across gcc
source files.  Requiring one file to do things differently is certain
to break over time.

One thing you could do is have genconfig.c look for the predicate of
define_cond_exec, and use it as the value of
HAVE_conditional_execution.  I don't know how well that would work.

The quick solution would be to introduce a new target hook and change
every place that checks HAVE_conditional_execution to check that.

Ian

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Carrot Wei]

On Sat, Oct 17, 2009 at 9:21 AM, Ian Lance Taylor <iant@google.com> wrote:
> Carrot Wei <carrot@google.com> writes:
>
>> Index: ifcvt.c
>> ===================================================================
>> --- ifcvt.c     (revision 152888)
>> +++ ifcvt.c     (working copy)
>> @@ -21,13 +21,13 @@
>>  #include "config.h"
>>  #include "system.h"
>>  #include "coretypes.h"
>> +#include "insn-config.h"
>>  #include "tm.h"
>>
>>  #include "rtl.h"
>>  #include "regs.h"
>>  #include "function.h"
>>  #include "flags.h"
>> -#include "insn-config.h"
>>  #include "recog.h"
>>  #include "except.h"
>>  #include "hard-reg-set.h"
>
>
> No.  The order of the main #includes is fairly canonical across gcc
> source files.  Requiring one file to do things differently is certain
> to break over time.
>
> One thing you could do is have genconfig.c look for the predicate of
> define_cond_exec, and use it as the value of
> HAVE_conditional_execution.  I don't know how well that would work.

Current method in genconfig.c is looking for COND_EXEC expression. I've
grepped several targets, there are several cases have cond_exec but no
define_cond_exec. So use define_cond_exec may change their behavior.

Can we change the output of genconfig.c to

#ifndef HAVE_conditional_execution
#define HAVE_conditional_execution 1
#endif

So we can use customized version of HAVE_conditional_execution defined
in the target specific header file if we want. Otherwise genconfig.c will
generate one according to the contents of md file.

>
> The quick solution would be to introduce a new target hook and change
> every place that checks HAVE_conditional_execution to check that.

I'm afraid simply replacing all HAVE_conditional_execution with new target hook
may break other targets have HAVE_conditional_execution enabled.


thanks
Carrot

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Ian Lance Taylor]

Carrot Wei <carrot@google.com> writes:

> Current method in genconfig.c is looking for COND_EXEC expression. I've
> grepped several targets, there are several cases have cond_exec but no
> define_cond_exec. So use define_cond_exec may change their behavior.

I agree that those targets should not be changed.

Would it be possible for genconfig to simply || together the various
predicates for the various cond_exec insns?


> Can we change the output of genconfig.c to
>
> #ifndef HAVE_conditional_execution
> #define HAVE_conditional_execution 1
> #endif
>
> So we can use customized version of HAVE_conditional_execution defined
> in the target specific header file if we want. Otherwise genconfig.c will
> generate one according to the contents of md file.

No, I don't think that is a good idea.  Nothing else in gcc works that
way, and I see nothing to recommend it.  Don't think "how can I solve
this problem today for me."  Think "how can I solve this for all time
for all targets in a way that is easy to maintain."


>> The quick solution would be to introduce a new target hook and change
>> every place that checks HAVE_conditional_execution to check that.
>
> I'm afraid simply replacing all HAVE_conditional_execution with new target hook
> may break other targets have HAVE_conditional_execution enabled.

Just make the default value for the target hook return
HAVE_conditional_execution.

Ian

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Carrot Wei]

On Sat, Oct 17, 2009 at 2:01 PM, Ian Lance Taylor <iant@google.com> wrote:
> Carrot Wei <carrot@google.com> writes:
>
>> Current method in genconfig.c is looking for COND_EXEC expression. I've
>> grepped several targets, there are several cases have cond_exec but no
>> define_cond_exec. So use define_cond_exec may change their behavior.
>
> I agree that those targets should not be changed.
>
> Would it be possible for genconfig to simply || together the various
> predicates for the various cond_exec insns?
>
>
>> Can we change the output of genconfig.c to
>>
>> #ifndef HAVE_conditional_execution
>> #define HAVE_conditional_execution 1
>> #endif
>>
>> So we can use customized version of HAVE_conditional_execution defined
>> in the target specific header file if we want. Otherwise genconfig.c will
>> generate one according to the contents of md file.
>
> No, I don't think that is a good idea.  Nothing else in gcc works that
> way, and I see nothing to recommend it.  Don't think "how can I solve
> this problem today for me."  Think "how can I solve this for all time
> for all targets in a way that is easy to maintain."
>
I learned this method from the definition of MAX_INSNS_PER_SPLIT.  :)

>
>>> The quick solution would be to introduce a new target hook and change
>>> every place that checks HAVE_conditional_execution to check that.
>>
>> I'm afraid simply replacing all HAVE_conditional_execution with new target hook
>> may break other targets have HAVE_conditional_execution enabled.
>
> Just make the default value for the target hook return
> HAVE_conditional_execution.
>
This is just what I need.
Thank you, Ian!

Carrot

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Carrot Wei]

The new patch added a new target hook have_conditional_execution to do the same
work as original macro HAVE_conditional_execution. But now we can have more
flexibility to return different result according to different target mode at
runtime. For arm it returns true when the target is not thumb1.

All the usage of macro HAVE_conditional_execution has been changed to the new
target hook call.

Test:
This patch was applied to trunk GCC and tested on the arm emulator with newlib.
No new failure.


ChangeLog:
2009-10-19  Wei Guozhi  <carrot@google.com>

        PR target/41705
        * target.h (have_conditional_execution): Add a new target hook function.
        * target-def.h (TARGET_HAVE_CONDITIONAL_EXECUTION): Likewise.
        * targhooks.h (default_have_conditional_execution): Likewise.
        * targhooks.c (default_have_conditional_execution): Likewise.
        * doc/tm.texi (TARGET_HAVE_CONDITIONAL_EXECUTION): Document it.
        * config/arm/arm.c (TARGET_HAVE_CONDITIONAL_EXECUTION): Define it.
        (arm_have_conditional_execution): New function.
        * config/arm/arm-protos.h (arm_have_conditional_execution):
        New function.
        * ifcvt.c (noce_process_if_block, find_if_header,
        cond_exec_find_if_block, dead_or_predicable): Change the usage of macro
        HAVE_conditional_execution to a target hook call.
        * recog.c (peephole2_optimize): Likewise.
        * sched-rgn.c (add_branch_dependences): Likewise.
        * final.c (asm_insn_count, final_scan_insn): Likewise.
        * bb-reorder.c (HAVE_conditional_execution): Remove it.

thanks
Guozhi



Index: target.h
===================================================================
--- target.h    (revision 152888)
+++ target.h    (working copy)
@@ -615,6 +615,9 @@ struct gcc_target
      already been generated.  */
   bool (* branch_target_register_callee_saved) (bool after_pe_gen);

+  /* Return true if the target supports conditional execution.  */
+  bool (* have_conditional_execution) (void);
+
   /* True if the constant X cannot be placed in the constant pool.  */
   bool (* cannot_force_const_mem) (rtx);


Index: target-def.h
===================================================================
--- target-def.h        (revision 152888)
+++ target-def.h        (working copy)
@@ -492,6 +492,7 @@
 #define TARGET_BRANCH_TARGET_REGISTER_CLASS \
   default_branch_target_register_class
 #define TARGET_BRANCH_TARGET_REGISTER_CALLEE_SAVED hook_bool_bool_false
+#define TARGET_HAVE_CONDITIONAL_EXECUTION default_have_conditional_execution
 #define TARGET_CANNOT_FORCE_CONST_MEM hook_bool_rtx_false
 #define TARGET_CANNOT_COPY_INSN_P NULL
 #define TARGET_COMMUTATIVE_P hook_bool_const_rtx_commutative_p
@@ -892,6 +893,7 @@
   TARGET_CANNOT_MODIFY_JUMPS_P,                        \
   TARGET_BRANCH_TARGET_REGISTER_CLASS,         \
   TARGET_BRANCH_TARGET_REGISTER_CALLEE_SAVED,  \
+  TARGET_HAVE_CONDITIONAL_EXECUTION,           \
   TARGET_CANNOT_FORCE_CONST_MEM,               \
   TARGET_CANNOT_COPY_INSN_P,                   \
   TARGET_COMMUTATIVE_P,                                \


Index: targhooks.h
===================================================================
--- targhooks.h (revision 152888)
+++ targhooks.h (working copy)
@@ -120,3 +120,4 @@ extern bool default_target_option_valid_
 extern bool default_target_option_pragma_parse (tree, tree);
 extern bool default_target_can_inline_p (tree, tree);
 extern unsigned int default_case_values_threshold (void);
+extern bool default_have_conditional_execution (void);


Index: targhooks.c
===================================================================
--- targhooks.c (revision 152888)
+++ targhooks.c (working copy)
@@ -892,4 +892,13 @@ unsigned int default_case_values_thresho
   return (HAVE_casesi ? 4 : 5);
 }

+bool default_have_conditional_execution (void)
+{
+#ifdef HAVE_conditional_execution
+  return HAVE_conditional_execution;
+#else
+  return false;
+#endif
+}
+
 #include "gt-targhooks.h"


Index: doc/tm.texi
===================================================================
--- doc/tm.texi (revision 152888)
+++ doc/tm.texi (working copy)
@@ -10733,6 +10733,12 @@ to have to make special provisions in @c
 to reserve space for caller-saved target registers.
 @end deftypefn

+@deftypefn {Target Hook} bool TARGET_HAVE_CONDITIONAL_EXECUTION (void)
+This target hook returns true if the target supports conditional execution.
+This target hook is required only when the target has several different
+modes and they have different conditional execution capability, such as ARM.
+@end deftypefn
+
 @defmac POWI_MAX_MULTS
 If defined, this macro is interpreted as a signed integer C expression
 that specifies the maximum number of floating point multiplications


Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c    (revision 152888)
+++ config/arm/arm.c    (working copy)
@@ -445,6 +445,9 @@ static const struct attribute_spec arm_a
 #define TARGET_HAVE_TLS true
 #endif

+#undef TARGET_HAVE_CONDITIONAL_EXECUTION
+#define TARGET_HAVE_CONDITIONAL_EXECUTION arm_have_conditional_execution
+
 #undef TARGET_CANNOT_FORCE_CONST_MEM
 #define TARGET_CANNOT_FORCE_CONST_MEM arm_cannot_force_const_mem

@@ -21173,4 +21176,12 @@ arm_frame_pointer_required (void)
           || (TARGET_ARM && TARGET_APCS_FRAME && ! leaf_function_p ()));
 }

+/* Only thumb1 can't support conditional execution, so return true if
+   the target is not thumb1.  */
+bool
+arm_have_conditional_execution (void)
+{
+  return !TARGET_THUMB1;
+}
+
 #include "gt-arm.h"


Index: config/arm/arm-protos.h
===================================================================
--- config/arm/arm-protos.h     (revision 152888)
+++ config/arm/arm-protos.h     (working copy)
@@ -41,7 +41,8 @@ extern HOST_WIDE_INT thumb_compute_initi
                                                               unsigned int);
 extern unsigned int arm_dbx_register_number (unsigned int);
 extern void arm_output_fn_unwind (FILE *, bool);
-
+extern bool arm_have_conditional_execution (void);
+

 #ifdef RTX_CODE
 extern bool arm_vector_mode_supported_p (enum machine_mode);
@@ -146,7 +147,8 @@ extern const char *vfp_output_fstmd (rtx
 extern void arm_set_return_address (rtx, rtx);
 extern int arm_eliminable_register (rtx);
 extern const char *arm_output_shift(rtx *, int);
-
+extern bool removable_cmp_0 (rtx cmp_op);
+extern const char *emit_branch_after_movs (rtx *operands);
 extern bool arm_output_addr_const_extra (FILE *, rtx);

 #if defined TREE_CODE


Index: ifcvt.c
===================================================================
--- ifcvt.c     (revision 152888)
+++ ifcvt.c     (working copy)
@@ -47,9 +47,6 @@
 #include "vecprim.h"
 #include "dbgcnt.h"

-#ifndef HAVE_conditional_execution
-#define HAVE_conditional_execution 0
-#endif
 #ifndef HAVE_conditional_move
 #define HAVE_conditional_move 0
 #endif
@@ -2419,7 +2416,7 @@ noce_process_if_block (struct noce_if_in
   if (HAVE_conditional_move
       && noce_try_cmove (if_info))
     goto success;
-  if (! HAVE_conditional_execution)
+  if (! targetm.have_conditional_execution ())
     {
       if (noce_try_store_flag_constants (if_info))
        goto success;
@@ -3063,7 +3060,7 @@ find_if_header (basic_block test_bb, int
       && noce_find_if_block (test_bb, then_edge, else_edge, pass))
     goto success;

-  if (HAVE_conditional_execution && reload_completed
+  if (targetm.have_conditional_execution () && reload_completed
       && cond_exec_find_if_block (&ce_info))
     goto success;

@@ -3073,7 +3070,7 @@ find_if_header (basic_block test_bb, int
     goto success;

   if (dom_info_state (CDI_POST_DOMINATORS) >= DOM_NO_FAST_QUERY
-      && (! HAVE_conditional_execution || reload_completed))
+      && (! targetm.have_conditional_execution () || reload_completed))
     {
       if (find_if_case_1 (test_bb, then_edge, else_edge))
        goto success;
@@ -3180,7 +3177,7 @@ cond_exec_find_if_block (struct ce_if_bl

   /* We only ever should get here after reload,
      and only if we have conditional execution.  */
-  gcc_assert (HAVE_conditional_execution && reload_completed);
+  gcc_assert (targetm.have_conditional_execution () && reload_completed);

   /* Discover if any fall through predecessors of the current test basic block
      were && tests (which jump to the else block) or || tests (which jump to
@@ -3858,7 +3855,7 @@ dead_or_predicable (basic_block test_bb,
   /* Disable handling dead code by conditional execution if the machine needs
      to do anything funny with the tests, etc.  */
 #ifndef IFCVT_MODIFY_TESTS
-  if (HAVE_conditional_execution)
+  if (targetm.have_conditional_execution ())
     {
       /* In the conditional execution case, we have things easy.  We know
         the condition is reversible.  We don't have to check life info


Index: recog.c
===================================================================
--- recog.c     (revision 152888)
+++ recog.c     (working copy)
@@ -3267,40 +3267,44 @@ peephole2_optimize (void)
                      do_cleanup_cfg |= purge_dead_edges (bb);
                    }

-#ifdef HAVE_conditional_execution
-                 for (i = 0; i < MAX_INSNS_PER_PEEP2 + 1; ++i)
-                   peep2_insn_data[i].insn = NULL_RTX;
-                 peep2_insn_data[peep2_current].insn = PEEP2_EOB;
-                 peep2_current_count = 0;
-#else
-                 /* Back up lifetime information past the end of the
-                    newly created sequence.  */
-                 if (++i >= MAX_INSNS_PER_PEEP2 + 1)
-                   i = 0;
-                 bitmap_copy (live, peep2_insn_data[i].live_before);
-
-                 /* Update life information for the new sequence.  */
-                 x = attempt;
-                 do
-                   {
-                     if (INSN_P (x))
-                       {
-                         if (--i < 0)
-                           i = MAX_INSNS_PER_PEEP2;
-                         if (peep2_current_count < MAX_INSNS_PER_PEEP2
-                             && peep2_insn_data[i].insn == NULL_RTX)
-                           peep2_current_count++;
-                         peep2_insn_data[i].insn = x;
-                         df_insn_rescan (x);
-                         df_simulate_one_insn_backwards (bb, x, live);
-                         bitmap_copy (peep2_insn_data[i].live_before, live);
-                       }
-                     x = PREV_INSN (x);
-                   }
-                 while (x != prev);
+                  if (targetm.have_conditional_execution ())
+                    {
+                      for (i = 0; i < MAX_INSNS_PER_PEEP2 + 1; ++i)
+                        peep2_insn_data[i].insn = NULL_RTX;
+                      peep2_insn_data[peep2_current].insn = PEEP2_EOB;
+                      peep2_current_count = 0;
+                    }
+                  else
+                    {
+                      /* Back up lifetime information past the end of the
+                         newly created sequence.  */
+                      if (++i >= MAX_INSNS_PER_PEEP2 + 1)
+                        i = 0;
+                      bitmap_copy (live, peep2_insn_data[i].live_before);
+
+                      /* Update life information for the new sequence.  */
+                      x = attempt;
+                      do
+                        {
+                          if (INSN_P (x))
+                            {
+                              if (--i < 0)
+                                i = MAX_INSNS_PER_PEEP2;
+                              if (peep2_current_count < MAX_INSNS_PER_PEEP2
+                                  && peep2_insn_data[i].insn == NULL_RTX)
+                                peep2_current_count++;
+                              peep2_insn_data[i].insn = x;
+                              df_insn_rescan (x);
+                              df_simulate_one_insn_backwards (bb, x, live);
+                              bitmap_copy (peep2_insn_data[i].live_before,
+                                           live);
+                            }
+                          x = PREV_INSN (x);
+                        }
+                      while (x != prev);

-                 peep2_current = i;
-#endif
+                      peep2_current = i;
+                    }

                  /* If we generated a jump instruction, it won't have
                     JUMP_LABEL set.  Recompute after we're done.  */


Index: sched-rgn.c
===================================================================
--- sched-rgn.c (revision 152888)
+++ sched-rgn.c (working copy)
@@ -2508,7 +2508,9 @@ add_branch_dependences (rtx head, rtx ta
          add_dependence (last, insn, REG_DEP_ANTI);
       }

-#ifdef HAVE_conditional_execution
+  if (!targetm.have_conditional_execution ())
+    return;
+
   /* Finally, if the block ends in a jump, and we are doing intra-block
      scheduling, make sure that the branch depends on any COND_EXEC insns
      inside the block to avoid moving the COND_EXECs past the branch insn.
@@ -2557,7 +2559,6 @@ add_branch_dependences (rtx head, rtx ta
       if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == COND_EXEC)
        add_dependence (tail, insn, REG_DEP_ANTI);
     }
-#endif
 }


Index: final.c
===================================================================
--- final.c     (revision 152888)
+++ final.c     (working copy)
@@ -204,10 +204,8 @@ rtx final_sequence;
 static int dialect_number;
 #endif

-#ifdef HAVE_conditional_execution
 /* Nonnull if the insn currently being emitted was a COND_EXEC pattern.  */
 rtx current_insn_predicate;
-#endif

 #ifdef HAVE_ATTR_length
 static int asm_insn_count (rtx);
@@ -2102,10 +2100,9 @@ final_scan_insn (rtx insn, FILE *file, i
        const char *templ;
        bool is_stmt;

-#ifdef HAVE_conditional_execution
        /* Reset this early so it is correct for ASM statements.  */
        current_insn_predicate = NULL_RTX;
-#endif
+
        /* An INSN, JUMP_INSN or CALL_INSN.
           First check for special kinds that recog doesn't recognize.  */

@@ -2590,10 +2587,9 @@ final_scan_insn (rtx insn, FILE *file, i
        FINAL_PRESCAN_INSN (insn, recog_data.operand, recog_data.n_operands);
 #endif

-#ifdef HAVE_conditional_execution
-       if (GET_CODE (PATTERN (insn)) == COND_EXEC)
+       if (targetm.have_conditional_execution ()
+            && GET_CODE (PATTERN (insn)) == COND_EXEC)
          current_insn_predicate = COND_EXEC_TEST (PATTERN (insn));
-#endif

 #ifdef HAVE_cc0
        cc_prev_status = cc_status;


Index: bb-reorder.c
===================================================================
--- bb-reorder.c        (revision 152888)
+++ bb-reorder.c        (working copy)
@@ -86,10 +86,6 @@
 #include "tree-pass.h"
 #include "df.h"

-#ifndef HAVE_conditional_execution
-#define HAVE_conditional_execution 0
-#endif
-
 /* The number of rounds.  In most cases there will only be 4 rounds, but
    when partitioning hot and cold basic blocks into separate sections of
    the .o file there will be an extra round.*/



On Sat, Oct 17, 2009 at 2:01 PM, Ian Lance Taylor <iant@google.com> wrote:
>>> The quick solution would be to introduce a new target hook and change
>>> every place that checks HAVE_conditional_execution to check that.
>>
>> I'm afraid simply replacing all HAVE_conditional_execution with new target hook
>> may break other targets have HAVE_conditional_execution enabled.
>
> Just make the default value for the target hook return
> HAVE_conditional_execution.
>
> Ian
>

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Ian Lance Taylor]

Carrot Wei <carrot@google.com> writes:

> +bool default_have_conditional_execution (void)
> +{

Line break after "bool".


> +/* Only thumb1 can't support conditional execution, so return true if
> +   the target is not thumb1.  */
> +bool
> +arm_have_conditional_execution (void)
> +{
> +  return !TARGET_THUMB1;
> +}
> +

Make this function static.  Add a static declaration at the top of the
file near the other static declarations.  Define is higher in the
file, perhaps around the cost functions.


> Index: config/arm/arm-protos.h
> ===================================================================
> --- config/arm/arm-protos.h     (revision 152888)
> +++ config/arm/arm-protos.h     (working copy)
> @@ -41,7 +41,8 @@ extern HOST_WIDE_INT thumb_compute_initi
>                                                                unsigned int);
>  extern unsigned int arm_dbx_register_number (unsigned int);
>  extern void arm_output_fn_unwind (FILE *, bool);
> -
> +extern bool arm_have_conditional_execution (void);

Do not declare this here.


> @@ -146,7 +147,8 @@ extern const char *vfp_output_fstmd (rtx
>  extern void arm_set_return_address (rtx, rtx);
>  extern int arm_eliminable_register (rtx);
>  extern const char *arm_output_shift(rtx *, int);
> -
> +extern bool removable_cmp_0 (rtx cmp_op);
> +extern const char *emit_branch_after_movs (rtx *operands);
>  extern bool arm_output_addr_const_extra (FILE *, rtx);

This seems like an unrelated change which should not be in this patch.


Please send a new patch.  Please also test the new patch on x86 or
x86_64 GNU/Linux.

I would like to hear if the ARM maintainers have any comments.

Thanks.

Ian

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Richard Earnshaw]

On Wed, 2009-10-21 at 20:52 -0700, Ian Lance Taylor wrote:
> Carrot Wei <carrot@google.com> writes:
> 
> > +bool default_have_conditional_execution (void)
> > +{
> 
> Line break after "bool".
> 
> 
> > +/* Only thumb1 can't support conditional execution, so return true if
> > +   the target is not thumb1.  */
> > +bool
> > +arm_have_conditional_execution (void)
> > +{
> > +  return !TARGET_THUMB1;
> > +}
> > +
> 
> Make this function static.  Add a static declaration at the top of the
> file near the other static declarations.  Define is higher in the
> file, perhaps around the cost functions.
> 
> 
> > Index: config/arm/arm-protos.h
> > ===================================================================
> > --- config/arm/arm-protos.h     (revision 152888)
> > +++ config/arm/arm-protos.h     (working copy)
> > @@ -41,7 +41,8 @@ extern HOST_WIDE_INT thumb_compute_initi
> >                                                                unsigned int);
> >  extern unsigned int arm_dbx_register_number (unsigned int);
> >  extern void arm_output_fn_unwind (FILE *, bool);
> > -
> > +extern bool arm_have_conditional_execution (void);
> 
> Do not declare this here.
> 
> 
> > @@ -146,7 +147,8 @@ extern const char *vfp_output_fstmd (rtx
> >  extern void arm_set_return_address (rtx, rtx);
> >  extern int arm_eliminable_register (rtx);
> >  extern const char *arm_output_shift(rtx *, int);
> > -
> > +extern bool removable_cmp_0 (rtx cmp_op);
> > +extern const char *emit_branch_after_movs (rtx *operands);
> >  extern bool arm_output_addr_const_extra (FILE *, rtx);
> 
> This seems like an unrelated change which should not be in this patch.
> 
> 
> Please send a new patch.  Please also test the new patch on x86 or
> x86_64 GNU/Linux.
> 
> I would like to hear if the ARM maintainers have any comments.
> 

This looks OK to me with the changes Ian has requested.  However, it
causes code that was previously conditionalized on
HAVE_conditional_execution to be unconditionally compiled, so please
also make sure that the code builds cleanly for at least one target that
does not define that macro.

R.

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Carrot Wei]

Changes done.

X86 is such a target that doesn't define the macro HAVE_conditional_execution.
I did the following testing without new failure:
Bootstrap on x86_64.
Gcc regression test on x86_64.
Gcc regression test on arm emulator with newlib.


ChangeLog:
2009-10-25  Wei Guozhi  <carrot@google.com>

        PR target/41705
        * target.h (have_conditional_execution): Add a new target hook function.
        * target-def.h (TARGET_HAVE_CONDITIONAL_EXECUTION): Likewise.
        * targhooks.h (default_have_conditional_execution): Likewise.
        * targhooks.c (default_have_conditional_execution): Likewise.
        * doc/tm.texi (TARGET_HAVE_CONDITIONAL_EXECUTION): Document it.
        * config/arm/arm.c (TARGET_HAVE_CONDITIONAL_EXECUTION): Define it.
        (arm_have_conditional_execution): New function.
        * ifcvt.c (noce_process_if_block, find_if_header,
        cond_exec_find_if_block, dead_or_predicable): Change the usage of macro
        HAVE_conditional_execution to a target hook call.
        * recog.c (peephole2_optimize): Likewise.
        * sched-rgn.c (add_branch_dependences): Likewise.
        * final.c (asm_insn_count, final_scan_insn): Likewise.
        * bb-reorder.c (HAVE_conditional_execution): Remove it.

thanks
Wei Guozhi


Index: target.h
===================================================================
--- target.h    (revision 152888)
+++ target.h    (working copy)
@@ -615,6 +615,9 @@ struct gcc_target
      already been generated.  */
   bool (* branch_target_register_callee_saved) (bool after_pe_gen);

+  /* Return true if the target supports conditional execution.  */
+  bool (* have_conditional_execution) (void);
+
   /* True if the constant X cannot be placed in the constant pool.  */
   bool (* cannot_force_const_mem) (rtx);


Index: target-def.h
===================================================================
--- target-def.h        (revision 152888)
+++ target-def.h        (working copy)
@@ -492,6 +492,7 @@
 #define TARGET_BRANCH_TARGET_REGISTER_CLASS \
   default_branch_target_register_class
 #define TARGET_BRANCH_TARGET_REGISTER_CALLEE_SAVED hook_bool_bool_false
+#define TARGET_HAVE_CONDITIONAL_EXECUTION default_have_conditional_execution
 #define TARGET_CANNOT_FORCE_CONST_MEM hook_bool_rtx_false
 #define TARGET_CANNOT_COPY_INSN_P NULL
 #define TARGET_COMMUTATIVE_P hook_bool_const_rtx_commutative_p
@@ -892,6 +893,7 @@
   TARGET_CANNOT_MODIFY_JUMPS_P,                        \
   TARGET_BRANCH_TARGET_REGISTER_CLASS,         \
   TARGET_BRANCH_TARGET_REGISTER_CALLEE_SAVED,  \
+  TARGET_HAVE_CONDITIONAL_EXECUTION,           \
   TARGET_CANNOT_FORCE_CONST_MEM,               \
   TARGET_CANNOT_COPY_INSN_P,                   \
   TARGET_COMMUTATIVE_P,                                \


Index: targhooks.h
===================================================================
--- targhooks.h (revision 152888)
+++ targhooks.h (working copy)
@@ -120,3 +120,4 @@ extern bool default_target_option_valid_
 extern bool default_target_option_pragma_parse (tree, tree);
 extern bool default_target_can_inline_p (tree, tree);
 extern unsigned int default_case_values_threshold (void);
+extern bool default_have_conditional_execution (void);


Index: targhooks.c
===================================================================
--- targhooks.c (revision 152888)
+++ targhooks.c (working copy)
@@ -892,4 +892,14 @@ unsigned int default_case_values_thresho
   return (HAVE_casesi ? 4 : 5);
 }

+bool
+default_have_conditional_execution (void)
+{
+#ifdef HAVE_conditional_execution
+  return HAVE_conditional_execution;
+#else
+  return false;
+#endif
+}
+
 #include "gt-targhooks.h"


Index: tm.texi
===================================================================
--- tm.texi     (revision 152888)
+++ tm.texi     (working copy)
@@ -10733,6 +10733,12 @@ to have to make special provisions in @c
 to reserve space for caller-saved target registers.
 @end deftypefn

+@deftypefn {Target Hook} bool TARGET_HAVE_CONDITIONAL_EXECUTION (void)
+This target hook returns true if the target supports conditional execution.
+This target hook is required only when the target has several different
+modes and they have different conditional execution capability, such as ARM.
+@end deftypefn
+
 @defmac POWI_MAX_MULTS
 If defined, this macro is interpreted as a signed integer C expression
 that specifies the maximum number of floating point multiplications


Index: arm.c
===================================================================
--- arm.c       (revision 152888)
+++ arm.c       (working copy)
@@ -138,6 +138,7 @@ static rtx arm_libcall_value (enum machi
 static void arm_internal_label (FILE *, const char *, unsigned long);
 static void arm_output_mi_thunk (FILE *, tree, HOST_WIDE_INT, HOST_WIDE_INT,
                                 tree);
+static bool arm_have_conditional_execution (void);
 static bool arm_rtx_costs_1 (rtx, enum rtx_code, int*, bool);
 static bool arm_size_rtx_costs (rtx, enum rtx_code, enum rtx_code, int *);
 static bool arm_slowmul_rtx_costs (rtx, enum rtx_code, enum rtx_code,
int *, bool);
@@ -445,6 +446,9 @@ static const struct attribute_spec arm_a
 #define TARGET_HAVE_TLS true
 #endif

+#undef TARGET_HAVE_CONDITIONAL_EXECUTION
+#define TARGET_HAVE_CONDITIONAL_EXECUTION arm_have_conditional_execution
+
 #undef TARGET_CANNOT_FORCE_CONST_MEM
 #define TARGET_CANNOT_FORCE_CONST_MEM arm_cannot_force_const_mem

@@ -21173,4 +21177,12 @@ arm_frame_pointer_required (void)
           || (TARGET_ARM && TARGET_APCS_FRAME && ! leaf_function_p ()));
 }

+/* Only thumb1 can't support conditional execution, so return true if
+   the target is not thumb1.  */
+static bool
+arm_have_conditional_execution (void)
+{
+  return !TARGET_THUMB1;
+}
+
 #include "gt-arm.h"


Index: ifcvt.c
===================================================================
--- ifcvt.c     (revision 152888)
+++ ifcvt.c     (working copy)
@@ -47,9 +47,6 @@
 #include "vecprim.h"
 #include "dbgcnt.h"

-#ifndef HAVE_conditional_execution
-#define HAVE_conditional_execution 0
-#endif
 #ifndef HAVE_conditional_move
 #define HAVE_conditional_move 0
 #endif
@@ -2419,7 +2416,7 @@ noce_process_if_block (struct noce_if_in
   if (HAVE_conditional_move
       && noce_try_cmove (if_info))
     goto success;
-  if (! HAVE_conditional_execution)
+  if (! targetm.have_conditional_execution ())
     {
       if (noce_try_store_flag_constants (if_info))
        goto success;
@@ -3063,7 +3060,7 @@ find_if_header (basic_block test_bb, int
       && noce_find_if_block (test_bb, then_edge, else_edge, pass))
     goto success;

-  if (HAVE_conditional_execution && reload_completed
+  if (targetm.have_conditional_execution () && reload_completed
       && cond_exec_find_if_block (&ce_info))
     goto success;

@@ -3073,7 +3070,7 @@ find_if_header (basic_block test_bb, int
     goto success;

   if (dom_info_state (CDI_POST_DOMINATORS) >= DOM_NO_FAST_QUERY
-      && (! HAVE_conditional_execution || reload_completed))
+      && (! targetm.have_conditional_execution () || reload_completed))
     {
       if (find_if_case_1 (test_bb, then_edge, else_edge))
        goto success;
@@ -3180,7 +3177,7 @@ cond_exec_find_if_block (struct ce_if_bl

   /* We only ever should get here after reload,
      and only if we have conditional execution.  */
-  gcc_assert (HAVE_conditional_execution && reload_completed);
+  gcc_assert (targetm.have_conditional_execution () && reload_completed);

   /* Discover if any fall through predecessors of the current test basic block
      were && tests (which jump to the else block) or || tests (which jump to
@@ -3858,7 +3855,7 @@ dead_or_predicable (basic_block test_bb,
   /* Disable handling dead code by conditional execution if the machine needs
      to do anything funny with the tests, etc.  */
 #ifndef IFCVT_MODIFY_TESTS
-  if (HAVE_conditional_execution)
+  if (targetm.have_conditional_execution ())
     {
       /* In the conditional execution case, we have things easy.  We know
         the condition is reversible.  We don't have to check life info


Index: recog.c
===================================================================
--- recog.c     (revision 152888)
+++ recog.c     (working copy)
@@ -3267,40 +3267,44 @@ peephole2_optimize (void)
                      do_cleanup_cfg |= purge_dead_edges (bb);
                    }

-#ifdef HAVE_conditional_execution
-                 for (i = 0; i < MAX_INSNS_PER_PEEP2 + 1; ++i)
-                   peep2_insn_data[i].insn = NULL_RTX;
-                 peep2_insn_data[peep2_current].insn = PEEP2_EOB;
-                 peep2_current_count = 0;
-#else
-                 /* Back up lifetime information past the end of the
-                    newly created sequence.  */
-                 if (++i >= MAX_INSNS_PER_PEEP2 + 1)
-                   i = 0;
-                 bitmap_copy (live, peep2_insn_data[i].live_before);
-
-                 /* Update life information for the new sequence.  */
-                 x = attempt;
-                 do
-                   {
-                     if (INSN_P (x))
-                       {
-                         if (--i < 0)
-                           i = MAX_INSNS_PER_PEEP2;
-                         if (peep2_current_count < MAX_INSNS_PER_PEEP2
-                             && peep2_insn_data[i].insn == NULL_RTX)
-                           peep2_current_count++;
-                         peep2_insn_data[i].insn = x;
-                         df_insn_rescan (x);
-                         df_simulate_one_insn_backwards (bb, x, live);
-                         bitmap_copy (peep2_insn_data[i].live_before, live);
-                       }
-                     x = PREV_INSN (x);
-                   }
-                 while (x != prev);
+                  if (targetm.have_conditional_execution ())
+                    {
+                      for (i = 0; i < MAX_INSNS_PER_PEEP2 + 1; ++i)
+                        peep2_insn_data[i].insn = NULL_RTX;
+                      peep2_insn_data[peep2_current].insn = PEEP2_EOB;
+                      peep2_current_count = 0;
+                    }
+                  else
+                    {
+                      /* Back up lifetime information past the end of the
+                         newly created sequence.  */
+                      if (++i >= MAX_INSNS_PER_PEEP2 + 1)
+                        i = 0;
+                      bitmap_copy (live, peep2_insn_data[i].live_before);
+
+                      /* Update life information for the new sequence.  */
+                      x = attempt;
+                      do
+                        {
+                          if (INSN_P (x))
+                            {
+                              if (--i < 0)
+                                i = MAX_INSNS_PER_PEEP2;
+                              if (peep2_current_count < MAX_INSNS_PER_PEEP2
+                                  && peep2_insn_data[i].insn == NULL_RTX)
+                                peep2_current_count++;
+                              peep2_insn_data[i].insn = x;
+                              df_insn_rescan (x);
+                              df_simulate_one_insn_backwards (bb, x, live);
+                              bitmap_copy (peep2_insn_data[i].live_before,
+                                           live);
+                            }
+                          x = PREV_INSN (x);
+                        }
+                      while (x != prev);

-                 peep2_current = i;
-#endif
+                      peep2_current = i;
+                    }

                  /* If we generated a jump instruction, it won't have
                     JUMP_LABEL set.  Recompute after we're done.  */


Index: sched-rgn.c
===================================================================
--- sched-rgn.c (revision 152888)
+++ sched-rgn.c (working copy)
@@ -2508,7 +2508,9 @@ add_branch_dependences (rtx head, rtx ta
          add_dependence (last, insn, REG_DEP_ANTI);
       }

-#ifdef HAVE_conditional_execution
+  if (!targetm.have_conditional_execution ())
+    return;
+
   /* Finally, if the block ends in a jump, and we are doing intra-block
      scheduling, make sure that the branch depends on any COND_EXEC insns
      inside the block to avoid moving the COND_EXECs past the branch insn.
@@ -2557,7 +2559,6 @@ add_branch_dependences (rtx head, rtx ta
       if (INSN_P (insn) && GET_CODE (PATTERN (insn)) == COND_EXEC)
        add_dependence (tail, insn, REG_DEP_ANTI);
     }
-#endif
 }


Index: final.c
===================================================================
--- final.c     (revision 152888)
+++ final.c     (working copy)
@@ -204,10 +204,8 @@ rtx final_sequence;
 static int dialect_number;
 #endif

-#ifdef HAVE_conditional_execution
 /* Nonnull if the insn currently being emitted was a COND_EXEC pattern.  */
 rtx current_insn_predicate;
-#endif

 #ifdef HAVE_ATTR_length
 static int asm_insn_count (rtx);
@@ -2102,10 +2100,9 @@ final_scan_insn (rtx insn, FILE *file, i
        const char *templ;
        bool is_stmt;

-#ifdef HAVE_conditional_execution
        /* Reset this early so it is correct for ASM statements.  */
        current_insn_predicate = NULL_RTX;
-#endif
+
        /* An INSN, JUMP_INSN or CALL_INSN.
           First check for special kinds that recog doesn't recognize.  */

@@ -2590,10 +2587,9 @@ final_scan_insn (rtx insn, FILE *file, i
        FINAL_PRESCAN_INSN (insn, recog_data.operand, recog_data.n_operands);
 #endif

-#ifdef HAVE_conditional_execution
-       if (GET_CODE (PATTERN (insn)) == COND_EXEC)
+       if (targetm.have_conditional_execution ()
+            && GET_CODE (PATTERN (insn)) == COND_EXEC)
          current_insn_predicate = COND_EXEC_TEST (PATTERN (insn));
-#endif

 #ifdef HAVE_cc0
        cc_prev_status = cc_status;


Index: bb-reorder.c
===================================================================
--- bb-reorder.c        (revision 152888)
+++ bb-reorder.c        (working copy)
@@ -86,10 +86,6 @@
 #include "tree-pass.h"
 #include "df.h"

-#ifndef HAVE_conditional_execution
-#define HAVE_conditional_execution 0
-#endif
-
 /* The number of rounds.  In most cases there will only be 4 rounds, but
    when partitioning hot and cold basic blocks into separate sections of
    the .o file there will be an extra round.*/



On Sat, Oct 24, 2009 at 12:31 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
>
> On Wed, 2009-10-21 at 20:52 -0700, Ian Lance Taylor wrote:
>> Carrot Wei <carrot@google.com> writes:
>>
>> > +bool default_have_conditional_execution (void)
>> > +{
>>
>> Line break after "bool".
>>
>>
>> > +/* Only thumb1 can't support conditional execution, so return true if
>> > +   the target is not thumb1.  */
>> > +bool
>> > +arm_have_conditional_execution (void)
>> > +{
>> > +  return !TARGET_THUMB1;
>> > +}
>> > +
>>
>> Make this function static.  Add a static declaration at the top of the
>> file near the other static declarations.  Define is higher in the
>> file, perhaps around the cost functions.
>>
>>
>> > Index: config/arm/arm-protos.h
>> > ===================================================================
>> > --- config/arm/arm-protos.h     (revision 152888)
>> > +++ config/arm/arm-protos.h     (working copy)
>> > @@ -41,7 +41,8 @@ extern HOST_WIDE_INT thumb_compute_initi
>> >                                                                unsigned int);
>> >  extern unsigned int arm_dbx_register_number (unsigned int);
>> >  extern void arm_output_fn_unwind (FILE *, bool);
>> > -
>> > +extern bool arm_have_conditional_execution (void);
>>
>> Do not declare this here.
>>
>>
>> > @@ -146,7 +147,8 @@ extern const char *vfp_output_fstmd (rtx
>> >  extern void arm_set_return_address (rtx, rtx);
>> >  extern int arm_eliminable_register (rtx);
>> >  extern const char *arm_output_shift(rtx *, int);
>> > -
>> > +extern bool removable_cmp_0 (rtx cmp_op);
>> > +extern const char *emit_branch_after_movs (rtx *operands);
>> >  extern bool arm_output_addr_const_extra (FILE *, rtx);
>>
>> This seems like an unrelated change which should not be in this patch.
>>
>>
>> Please send a new patch.  Please also test the new patch on x86 or
>> x86_64 GNU/Linux.
>>
>> I would like to hear if the ARM maintainers have any comments.
>>
>
> This looks OK to me with the changes Ian has requested.  However, it
> causes code that was previously conditionalized on
> HAVE_conditional_execution to be unconditionally compiled, so please
> also make sure that the code builds cleanly for at least one target that
> does not define that macro.
>
> R.
>
>
>

Re: [PATCH: PR target/41705] Enable if conversion for thumb1 by new HAVE_conditional_execution definition

[Ian Lance Taylor]

Carrot Wei <carrot@google.com> writes:

> 2009-10-25  Wei Guozhi  <carrot@google.com>
>
>         PR target/41705
>         * target.h (have_conditional_execution): Add a new target hook function.
>         * target-def.h (TARGET_HAVE_CONDITIONAL_EXECUTION): Likewise.
>         * targhooks.h (default_have_conditional_execution): Likewise.
>         * targhooks.c (default_have_conditional_execution): Likewise.
>         * doc/tm.texi (TARGET_HAVE_CONDITIONAL_EXECUTION): Document it.
>         * config/arm/arm.c (TARGET_HAVE_CONDITIONAL_EXECUTION): Define it.
>         (arm_have_conditional_execution): New function.
>         * ifcvt.c (noce_process_if_block, find_if_header,
>         cond_exec_find_if_block, dead_or_predicable): Change the usage of macro
>         HAVE_conditional_execution to a target hook call.
>         * recog.c (peephole2_optimize): Likewise.
>         * sched-rgn.c (add_branch_dependences): Likewise.
>         * final.c (asm_insn_count, final_scan_insn): Likewise.
>         * bb-reorder.c (HAVE_conditional_execution): Remove it.

This is OK if nobody objects in the next 24 hours.

Thanks.

Ian