[PATCH 0/6] If conversion with multiple sets.

[PATCH 0/6] If conversion with multiple sets.

[Robin Dapp]

Hi,

the follow patch set was created in an attempt to allow multiple sets to be
if converted.  I was not able to make it work out of the box since I found the
cost estimation for the newly created sequence to always be much higher than
the sequence before.
 This is due to noce_convert_multiple_sets creating temporaries that will only
get optimized away (if at all) after the cost estimation.  Therefore, I decided
to expose the number of created conditional moves to the backend in the hope
that all temporaries get eliminated eventually.  The backend may still use the
cost estimation but currently, the original_cost is not even set up properly
when the noce_conversion_profitable_p is called.

The series also allows noce_convert_multiple_sets to use immediate operands
without moving them into a register.  Moreover it tries to only create
temporaries when needed so in the future, a cost estimation may be easier.

Regards
 Robin

--

  ifcvt: Store the number of created cmovs.
  ifcvt: Allow constants operands in noce_convert_multiple_sets.
  ifcvt: Use enum instead of transform_name string.
  S/390: Implement noce_conversion_profitable_p.
  ifcvt: Only created temporaries as needed.
  S/390: Add test for noce_convert_multiple_sets.

 gcc/config/s390/s390.c                        |  17 ++
 gcc/ifcvt.c                                   | 148 ++++++++++++++----
 gcc/ifcvt.h                                   |  71 ++++++++-
 .../gcc.target/s390/ifcvt-two-insns-int.c     |  26 +++
 4 files changed, 226 insertions(+), 36 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/s390/ifcvt-two-insns-int.c

-- 
2.17.0

[PATCH 1/6] ifcvt: Store the number of created cmovs.

[Robin Dapp]

This patch saves the number of created conditional moves by
noce_convert_multiple_sets in the IF_INFO struct.  This may be used by
the backend to easier decide whether to accept a generated sequence or
not.

--

gcc/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* ifcvt.c (noce_convert_multiple_sets): Set cmov count.
	(noce_find_if_block): Set cmov count.
	* ifcvt.h (struct noce_if_info): Add cmov count.
---
 gcc/ifcvt.c | 10 ++++++++--
 gcc/ifcvt.h |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 8b3907618e7..ddf077fa051 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3247,9 +3247,14 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
   /* Actually emit the sequence if it isn't too expensive.  */
   rtx_insn *seq = get_insns ();
 
+  if_info->transform_name = "noce_convert_multiple_sets";
+  if_info->created_cmovs = count;
+
   if (!targetm.noce_conversion_profitable_p (seq, if_info))
     {
       end_sequence ();
+      if_info->transform_name = "";
+      if_info->created_cmovs = 0;
       return FALSE;
     }
 
@@ -3296,7 +3301,7 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
     }
 
   num_updated_if_blocks++;
-  if_info->transform_name = "noce_convert_multiple_sets";
+
   return TRUE;
 }
 
@@ -4060,7 +4065,8 @@ noce_find_if_block (basic_block test_bb, edge then_edge, edge else_edge,
      and jump_insns are always given a cost of 1 by seq_cost, so treat
      both instructions as having cost COSTS_N_INSNS (1).  */
   if_info.original_cost = COSTS_N_INSNS (2);
-
+  if_info.transform_name = "";
+  if_info.created_cmovs = 0;
 
   /* Do the real work.  */
 
diff --git a/gcc/ifcvt.h b/gcc/ifcvt.h
index a18ba94b8df..50f40bbd1e5 100644
--- a/gcc/ifcvt.h
+++ b/gcc/ifcvt.h
@@ -108,6 +108,10 @@ struct noce_if_info
   /* The name of the noce transform that succeeded in if-converting
      this structure.  Used for debugging.  */
   const char *transform_name;
+
+  /* The number of created conditional moves in case we convert multiple
+     sets.  */
+  unsigned int created_cmovs;
 };
 
 #endif /* GCC_IFCVT_H */
-- 
2.17.0

Re: [PATCH 1/6] ifcvt: Store the number of created cmovs.

[Jeff Law]

On 11/14/18 6:07 AM, Robin Dapp wrote:
> This patch saves the number of created conditional moves by
> noce_convert_multiple_sets in the IF_INFO struct.  This may be used by
> the backend to easier decide whether to accept a generated sequence or
> not.
> 
> --
> 
> gcc/ChangeLog:
> 
> 2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>
> 
> 	* ifcvt.c (noce_convert_multiple_sets): Set cmov count.
> 	(noce_find_if_block): Set cmov count.
> 	* ifcvt.h (struct noce_if_info): Add cmov count.
So this series came in after stage1 close.    I'm not aware of a bug
this series is meant to fix, but perhaps you just didn't include a
reference to it.

Anyway, patches #1 and #3 are OK for the trunk right now.

Jeff

[PATCH 5/6] ifcvt: Only created temporaries as needed.

[Robin Dapp]

noce_convert_multiple_sets creates temporaries for the destination of
every emitted cmov and expects subsequent passes to get rid of them.  This
does not happen every time and even if the temporaries are removed, code
generation can be affected adversely.  In this patch, temporaries are
only created if the destination of a set is used in an emitted condition
check.

--

gcc/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* ifcvt.c (check_need_temps): New function.
	(noce_convert_multiple_sets): Only created temporaries if needed.
---
 gcc/ifcvt.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 94822c583fe..6d1803ed40d 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -99,6 +99,10 @@ static int dead_or_predicable (basic_block, basic_block, basic_block,
 			       edge, int);
 static void noce_emit_move_insn (rtx, rtx);
 static rtx_insn *block_has_only_trap (basic_block);
+static void check_need_temps (basic_block bb,
+                              hash_map<rtx, bool> *need_temp,
+                              rtx cond);
+
 \f
 /* Count the number of non-jump active insns in BB.  */
 
@@ -3166,6 +3170,12 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
   auto_vec<rtx_insn *> unmodified_insns;
   int count = 0;
 
+  hash_map<rtx, bool> need_temps;
+
+  check_need_temps (then_bb, &need_temps, cond);
+
+  hash_map<rtx, bool> temps_created;
+
   FOR_BB_INSNS (then_bb, insn)
     {
       /* Skip over non-insns.  */
@@ -3176,10 +3186,20 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
       gcc_checking_assert (set);
 
       rtx target = SET_DEST (set);
-      rtx temp = gen_reg_rtx (GET_MODE (target));
       rtx new_val = SET_SRC (set);
       rtx old_val = target;
 
+      rtx dest = SET_DEST (set);
+
+      rtx temp;
+      if (need_temps.get (dest))
+       {
+         temp = gen_reg_rtx (GET_MODE (target));
+         temps_created.put (target, true);
+       }
+      else
+       temp = target;
+
       /* If we were supposed to read from an earlier write in this block,
 	 we've changed the register allocation.  Rewire the read.  While
 	 we are looking, also try to catch a swap idiom.  */
@@ -3269,8 +3289,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
 
   /* Now fixup the assignments.  */
   for (int i = 0; i < count; i++)
-    noce_emit_move_insn (targets[i], temporaries[i]);
-
+    if (temps_created.get(targets[i]) && targets[i] != temporaries[i])
+      noce_emit_move_insn (targets[i], temporaries[i]);
 
   /* Actually emit the sequence if it isn't too expensive.  */
   rtx_insn *seq = get_insns ();
@@ -3775,6 +3795,34 @@ check_cond_move_block (basic_block bb,
   return TRUE;
 }
 
+/* Check for which sets we need to emit temporaries to hold the destination of
+   a conditional move.  */
+static void
+check_need_temps (basic_block bb, hash_map<rtx, bool> *need_temp, rtx cond)
+{
+  rtx_insn *insn;
+
+  FOR_BB_INSNS (bb, insn)
+    {
+      rtx set, dest;
+
+      if (!active_insn_p (insn))
+	continue;
+
+      set = single_set (insn);
+      if (set == NULL_RTX)
+	continue;
+
+      dest = SET_DEST (set);
+
+      /* noce_emit_cmove will emit the condition check every time it is called
+         so we need a temp register if the destination is modified.  */
+      if (reg_overlap_mentioned_p (dest, cond))
+	need_temp->put (dest, true);
+    }
+}
+
+
 /* Given a basic block BB suitable for conditional move conversion,
    a condition COND, and pointer maps THEN_VALS and ELSE_VALS containing
    the register values depending on COND, emit the insns in the block as
-- 
2.17.0

Re: [PATCH 5/6] ifcvt: Only created temporaries as needed.

[Jeff Law]

On 11/14/18 6:07 AM, Robin Dapp wrote:
> noce_convert_multiple_sets creates temporaries for the destination of
> every emitted cmov and expects subsequent passes to get rid of them.  This
> does not happen every time and even if the temporaries are removed, code
> generation can be affected adversely.  In this patch, temporaries are
> only created if the destination of a set is used in an emitted condition
> check.
> 
> --
> 
> gcc/ChangeLog:
> 
> 2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>
> 
> 	* ifcvt.c (check_need_temps): New function.
> 	(noce_convert_multiple_sets): Only created temporaries if needed.
This looks pretty reasonable.  ISTM it ought to be able to go forward if
it's tested independently.

jeff

Re: [PATCH 5/6] ifcvt: Only created temporaries as needed.

[Robin Dapp]

> This looks pretty reasonable.  ISTM it ought to be able to go forward if
> it's tested independently.

The test suite already passes, any other tests you have in mind?  To be
honest I suppose noce_convert_multiple_sets will currently never
successfully return (due to the costing problems I described before), so
a specific test case might be difficult.

Regards
 Robin

Re: [PATCH 5/6] ifcvt: Only created temporaries as needed.

[Jeff Law]

On 11/15/18 4:38 AM, Robin Dapp wrote:
>> This looks pretty reasonable.  ISTM it ought to be able to go forward if
>> it's tested independently.
> 
> The test suite already passes, any other tests you have in mind?  To be
> honest I suppose noce_convert_multiple_sets will currently never
> successfully return (due to the costing problems I described before), so
> a specific test case might be difficult.
There can be subtle dependencies on prior patches when you test it as
part of a set.  If we're going to gate this in independently, then it
should be tested independently.

Jeff

[PATCH 6/6] S/390: Add test for noce_convert_multiple_sets.

[Robin Dapp]

New test.

--

gcc/testsuite/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* gcc.target/s390/ifcvt-two-insns-int.c: New test.
---
 .../gcc.target/s390/ifcvt-two-insns-int.c     | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/s390/ifcvt-two-insns-int.c

diff --git a/gcc/testsuite/gcc.target/s390/ifcvt-two-insns-int.c b/gcc/testsuite/gcc.target/s390/ifcvt-two-insns-int.c
new file mode 100644
index 00000000000..952c8fd890e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/ifcvt-two-insns-int.c
@@ -0,0 +1,26 @@
+/* Check load on condition for bool.  */
+
+/* { dg-do compile { target { s390*-*-* } } } */
+/* { dg-options "-O2 -march=z13" } */
+
+/* { dg-final { scan-assembler "lochinhe\t%r.?,1" } } */
+/* { dg-final { scan-assembler "locrhe\t.*" } } */
+#include <stdbool.h>
+
+int foo (int *a, unsigned int n)
+{
+  int min = 999999;
+  int bla = 0;
+  for (int i = 0; i < n; i++)
+    {
+      if (a[i] < min)
+	{
+	  min = a[i];
+	  bla = 1;
+	}
+    }
+
+  if (bla)
+    min += 1;
+  return min;
+}
-- 
2.17.0

[PATCH 4/6] S/390: Implement noce_conversion_profitable_p.

[Robin Dapp]

This patch implements noce_conversion_profitable_p by checking for the
transformation ifcvt used and only return positively if
noce_convert_multiple_sets created less than MAX_IFCVT_INSNS insns.

--

gcc/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* config/s390/s390.c (MAX_IFCVT_INSNS): Define.
	(s390_noce_conversion_profitable_p): Implement.
	(TARGET_NOCE_CONVERSION_PROFITABLE_P): Define.
---
 gcc/config/s390/s390.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 0f33101d779..1018d9b8057 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -86,6 +86,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "ipa-prop.h"
 #include "ipa-fnsummary.h"
 #include "sched-int.h"
+#include "ifcvt.h"
 
 /* This file should be included last.  */
 #include "target-def.h"
@@ -393,6 +394,8 @@ struct s390_address
   bool literal_pool;
 };
 
+#define MAX_IFCVT_INSNS        2
+
 /* Few accessor macros for struct cfun->machine->s390_frame_layout.  */
 
 #define cfun_frame_layout (cfun->machine->frame_layout)
@@ -15989,6 +15992,17 @@ s390_case_values_threshold (void)
   return default_case_values_threshold ();
 }
 
+static bool
+s390_noce_conversion_profitable_p (rtx_insn *seq, struct noce_if_info *if_info)
+{
+  if (if_info->transform
+      && if_info->transform == ifcvt_transform_noce_convert_multiple_sets
+      && if_info->created_cmovs <= MAX_IFCVT_INSNS)
+    return true;
+
+  return default_noce_conversion_profitable_p (seq, if_info);
+}
+
 /* Initialize GCC target structure.  */
 
 #undef  TARGET_ASM_ALIGNED_HI_OP
@@ -16240,6 +16254,9 @@ s390_case_values_threshold (void)
 #undef TARGET_VECTORIZE_SUPPORT_VECTOR_MISALIGNMENT
 #define TARGET_VECTORIZE_SUPPORT_VECTOR_MISALIGNMENT s390_support_vector_misalignment
 
+#undef TARGET_NOCE_CONVERSION_PROFITABLE_P
+#define TARGET_NOCE_CONVERSION_PROFITABLE_P s390_noce_conversion_profitable_p
+
 #undef TARGET_VECTOR_ALIGNMENT
 #define TARGET_VECTOR_ALIGNMENT s390_vector_alignment
 
-- 
2.17.0

[PATCH 2/6] ifcvt: Allow constants operands in noce_convert_multiple_sets.

[Robin Dapp]

This patch checks whether the current target supports conditional moves
with immediate then/else operands and allows noce_convert_multiple_sets
to deal with constants subsequently.
Also, minor refactoring is performed.

--

gcc/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* ifcvt.c (have_const_cmov): New function.
	(noce_convert_multiple_sets): Allow constants if supported.
	(bb_ok_for_noce_convert_multiple_sets): Likewise.
	(check_cond_move_block): Refactor.
---
 gcc/ifcvt.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index ddf077fa051..660bb46eb1c 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3077,6 +3077,27 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond,
   return false;
 }
 
+/* Check if we have a movcc pattern that accepts constants as then/else
+   operand (op 2/3).  */
+static bool
+have_const_cmov (machine_mode mode)
+{
+  enum insn_code icode;
+  if ((icode = direct_optab_handler (movcc_optab, mode))
+      != CODE_FOR_nothing)
+    {
+      if (insn_data[(int) icode].operand[2].predicate
+	  && (insn_data[(int) icode].operand[2].predicate
+	    (const1_rtx, insn_data[(int) icode].operand[2].mode)))
+	if (insn_data[(int) icode].operand[3].predicate
+	    && (insn_data[(int) icode].operand[3].predicate
+	      (const1_rtx, insn_data[(int) icode].operand[3].mode)))
+	  return true;
+    }
+
+  return false;
+}
+
 /* We have something like:
 
      if (x > y)
@@ -3194,7 +3215,12 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
 	 we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
 	 Wrap the two cmove operands into subregs if appropriate to prevent
 	 that.  */
-      if (GET_MODE (new_val) != GET_MODE (temp))
+
+      /* Check if we can emit a cmove with constant operands.  */
+      bool allow_constants = have_const_cmov (GET_MODE (target));
+
+      if (!(allow_constants && CONST_INT_P (new_val))
+         && GET_MODE (new_val) != GET_MODE (temp))
 	{
 	  machine_mode src_mode = GET_MODE (new_val);
 	  machine_mode dst_mode = GET_MODE (temp);
@@ -3205,7 +3231,8 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
 	    }
 	  new_val = lowpart_subreg (dst_mode, new_val, src_mode);
 	}
-      if (GET_MODE (old_val) != GET_MODE (temp))
+      if (!(allow_constants && CONST_INT_P (old_val))
+         && GET_MODE (old_val) != GET_MODE (temp))
 	{
 	  machine_mode src_mode = GET_MODE (old_val);
 	  machine_mode dst_mode = GET_MODE (temp);
@@ -3339,9 +3366,10 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb)
       if (!REG_P (dest))
 	return false;
 
-      if (!(REG_P (src)
-	   || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))
-	       && subreg_lowpart_p (src))))
+      if (!((REG_P (src)
+	      || (have_const_cmov (GET_MODE (dest)) && CONST_INT_P (src)))
+	    || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))
+	      && subreg_lowpart_p (src))))
 	return false;
 
       /* Destination must be appropriate for a conditional write.  */
@@ -3689,7 +3717,7 @@ check_cond_move_block (basic_block bb,
     {
       rtx set, dest, src;
 
-      if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn))
+      if (!active_insn_p (insn))
 	continue;
       set = single_set (insn);
       if (!set)
@@ -3705,10 +3733,8 @@ check_cond_move_block (basic_block bb,
       if (!CONSTANT_P (src) && !register_operand (src, VOIDmode))
 	return FALSE;
 
-      if (side_effects_p (src) || side_effects_p (dest))
-	return FALSE;
-
-      if (may_trap_p (src) || may_trap_p (dest))
+      /* Check for side effects and trapping.  */
+      if (!noce_operand_ok (src) || !noce_operand_ok (dest))
 	return FALSE;
 
       /* Don't try to handle this if the source register was
-- 
2.17.0

Re: [PATCH 2/6] ifcvt: Allow constants operands in noce_convert_multiple_sets.

[Jeff Law]

On 11/14/18 6:07 AM, Robin Dapp wrote:
> This patch checks whether the current target supports conditional moves
> with immediate then/else operands and allows noce_convert_multiple_sets
> to deal with constants subsequently.
> Also, minor refactoring is performed.
> 
> --
> 
> gcc/ChangeLog:
> 
> 2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>
> 
> 	* ifcvt.c (have_const_cmov): New function.
> 	(noce_convert_multiple_sets): Allow constants if supported.
> 	(bb_ok_for_noce_convert_multiple_sets): Likewise.
> 	(check_cond_move_block): Refactor.
> ---
>  gcc/ifcvt.c | 46 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index ddf077fa051..660bb46eb1c 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -3077,6 +3077,27 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond,
>    return false;
>  }
>  
> +/* Check if we have a movcc pattern that accepts constants as then/else
> +   operand (op 2/3).  */
> +static bool
> +have_const_cmov (machine_mode mode)
> +{
> +  enum insn_code icode;
> +  if ((icode = direct_optab_handler (movcc_optab, mode))
> +      != CODE_FOR_nothing)
> +    {
> +      if (insn_data[(int) icode].operand[2].predicate
> +	  && (insn_data[(int) icode].operand[2].predicate
> +	    (const1_rtx, insn_data[(int) icode].operand[2].mode)))
> +	if (insn_data[(int) icode].operand[3].predicate
> +	    && (insn_data[(int) icode].operand[3].predicate
> +	      (const1_rtx, insn_data[(int) icode].operand[3].mode)))
> +	  return true;
> +    }
> +
> +  return false;
> +}
This may ultimately be too simplistic.  There are targets where some
constants are OK, but others may not be.   By checking the predicate
like this I think you can cause over-aggressive if-conversion if the
target allows a range of integers in the expander's operand predicate,
but allows a narrower range in the actual define_insn (presumably the
expander loads them into a pseudo or somesuch in that case).

We know that over-aggressive if-conversion into conditional moves hurts
some targets.

Ideally you'd create the actual insn with the constants you want to use
and see if that's recognized as well as compute its cost.  Is that just
too painful at this point for some reason?


> @@ -3689,7 +3717,7 @@ check_cond_move_block (basic_block bb,
>      {
>        rtx set, dest, src;
>  
> -      if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn))
> +      if (!active_insn_p (insn))
>  	continue;
>        set = single_set (insn);
>        if (!set)
> @@ -3705,10 +3733,8 @@ check_cond_move_block (basic_block bb,
>        if (!CONSTANT_P (src) && !register_operand (src, VOIDmode))
>  	return FALSE;
>  
> -      if (side_effects_p (src) || side_effects_p (dest))
> -	return FALSE;
> -
> -      if (may_trap_p (src) || may_trap_p (dest))
> +      /* Check for side effects and trapping.  */
> +      if (!noce_operand_ok (src) || !noce_operand_ok (dest))
>  	return FALSE;
>  
>        /* Don't try to handle this if the source register was
These two hunks are probably OK as general cleanups.  Note that
noce_operand_ok is not strictly the same as checking side_effects_p and
may_trap_p in the case of a MEM.  But you've already filtered out MEMs
before you get here.

Jeff
> 

Re: [PATCH 2/6] ifcvt: Allow constants operands in noce_convert_multiple_sets.

[Robin Dapp]

> This may ultimately be too simplistic.  There are targets where some
> constants are OK, but others may not be.   By checking the predicate
> like this I think you can cause over-aggressive if-conversion if the
> target allows a range of integers in the expander's operand predicate,
> but allows a narrower range in the actual define_insn (presumably the
> expander loads them into a pseudo or somesuch in that case).
> 
> We know that over-aggressive if-conversion into conditional moves hurts
> some targets.
> 
> Ideally you'd create the actual insn with the constants you want to use
> and see if that's recognized as well as compute its cost.  Is that just
> too painful at this point for some reason?

Conditional moves in noce_convert_multiple_sets are processed via
noce_emit_cmove which itself performs quite some preprocessing and calls
optab's emit_conditional_move in the end, so the insn will already be
emitted before being able to decide whether to decline it due to costs.
In addition, every noce_emit_cmove will emit the condition check again
as well as possible temporaries.

Comparing the costs of the whole sequence will therefore still prove
difficult as all the additionally generated insns will not get removed
until reload and make a fair comparison nigh impossible.

I was reluctant to factor out all the preprocessing stuff, separate it
from the condition check and actual emitting part but that's properly
the "right way" to do it, including emitting the condition only once in
the beginning.

However, for now, I could imagine changing only the
conversion_profitable_p hook in our backend to only count the cmovs and
ignore everything else in the sequence.  This would be somewhat hacky
though and still wouldn't allow constants :)

Regards
 Robin

Re: [PATCH 2/6] ifcvt: Allow constants operands in noce_convert_multiple_sets.

[Jeff Law]

On 11/15/18 4:34 AM, Robin Dapp wrote:
>> This may ultimately be too simplistic.  There are targets where some
>> constants are OK, but others may not be.   By checking the predicate
>> like this I think you can cause over-aggressive if-conversion if the
>> target allows a range of integers in the expander's operand predicate,
>> but allows a narrower range in the actual define_insn (presumably the
>> expander loads them into a pseudo or somesuch in that case).
>>
>> We know that over-aggressive if-conversion into conditional moves hurts
>> some targets.
>>
>> Ideally you'd create the actual insn with the constants you want to use
>> and see if that's recognized as well as compute its cost.  Is that just
>> too painful at this point for some reason?
> 
> Conditional moves in noce_convert_multiple_sets are processed via
> noce_emit_cmove which itself performs quite some preprocessing and calls
> optab's emit_conditional_move in the end, so the insn will already be
> emitted before being able to decide whether to decline it due to costs.
> In addition, every noce_emit_cmove will emit the condition check again
> as well as possible temporaries.
I wonder if we could arrange to emit them onto a new sequence.  We could
then cost the entire sequence.  If we end up wanting to do the
transformation we then splice that sequence into the main insn chain at
the appropriate place.

There should be a start/push/pop/end for sequences that you could use
for helpers.

> 
> Comparing the costs of the whole sequence will therefore still prove
> difficult as all the additionally generated insns will not get removed
> until reload and make a fair comparison nigh impossible.
You'd probably have to make an educated guess about what insns from the
original sequence would die and could therefore be removed.

> 
> I was reluctant to factor out all the preprocessing stuff, separate it
> from the condition check and actual emitting part but that's properly
> the "right way" to do it, including emitting the condition only once in
> the beginning.
If the push_to_sequence idea works you may not have to do a ton of
refactoring.  Essentially it changes where things like emit_insn and
friends splice in newly created insns.  It may allow you to leave most
of the current code in-place and you just have to splice in the sequence
at the end (there's a routine to help you with that too -- it's been a
long time, so the names aren't in memory anymore).


> 
> However, for now, I could imagine changing only the
> conversion_profitable_p hook in our backend to only count the cmovs and
> ignore everything else in the sequence.  This would be somewhat hacky
> though and still wouldn't allow constants :)
That does sound hacky..

jeff

[PATCH 3/6] ifcvt: Use enum instead of transform_name string.

[Robin Dapp]

This patch introduces an enum for ifcvt's various noce transformations.
As the transformation might be queried by the backend, I find it nicer
to allow checking for a proper type instead of a string comparison.

--

gcc/ChangeLog:

2018-11-14  Robin Dapp  <rdapp@linux.ibm.com>

	* ifcvt.c (noce_try_move): Use new function.
	(noce_try_ifelse_collapse): Likewise.
	(noce_try_store_flag): Likewise.
	(noce_try_inverse_constants): Likewise.
	(noce_try_store_flag_constants): Likewise.
	(noce_try_addcc): Likewise.
	(noce_try_store_flag_mask): Likewise.
	(noce_try_cmove): Likewise.
	(noce_try_cmove_arith): Likewise.
	(noce_try_minmax): Likewise.
	(noce_try_abs): Likewise.
	(noce_try_sign_mask): Likewise.
	(noce_try_bitop): Likewise.
	(noce_convert_multiple_sets): Likewise.
	(noce_process_if_block): Likewise.
	(noce_find_if_block): Likewise.
	* ifcvt.h (enum ifcvt_transform): Introduce enum.
	(ifcvt_get_transform_name): New function.
	(struct noce_if_info): Use enum instead of string.
---
 gcc/ifcvt.c | 46 ++++++++++++++++++------------------
 gcc/ifcvt.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 88 insertions(+), 25 deletions(-)

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 660bb46eb1c..94822c583fe 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1105,7 +1105,7 @@ noce_try_move (struct noce_if_info *if_info)
 	  emit_insn_before_setloc (seq, if_info->jump,
 				   INSN_LOCATION (if_info->insn_a));
 	}
-      if_info->transform_name = "noce_try_move";
+      if_info->transform = ifcvt_transform_noce_try_move;
       return TRUE;
     }
   return FALSE;
@@ -1139,7 +1139,7 @@ noce_try_ifelse_collapse (struct noce_if_info * if_info)
   emit_insn_before_setloc (seq, if_info->jump,
 			  INSN_LOCATION (if_info->insn_a));
 
-  if_info->transform_name = "noce_try_ifelse_collapse";
+  if_info->transform = ifcvt_transform_noce_try_ifelse_collapse;
   return TRUE;
 }
 
@@ -1186,7 +1186,7 @@ noce_try_store_flag (struct noce_if_info *if_info)
 
       emit_insn_before_setloc (seq, if_info->jump,
 			       INSN_LOCATION (if_info->insn_a));
-      if_info->transform_name = "noce_try_store_flag";
+      if_info->transform = ifcvt_transform_noce_try_store_flag;
       return TRUE;
     }
   else
@@ -1265,7 +1265,7 @@ noce_try_inverse_constants (struct noce_if_info *if_info)
 
       emit_insn_before_setloc (seq, if_info->jump,
 			       INSN_LOCATION (if_info->insn_a));
-      if_info->transform_name = "noce_try_inverse_constants";
+      if_info->transform = ifcvt_transform_noce_try_inverse_constants;
       return true;
     }
 
@@ -1485,7 +1485,7 @@ noce_try_store_flag_constants (struct noce_if_info *if_info)
 
       emit_insn_before_setloc (seq, if_info->jump,
 			       INSN_LOCATION (if_info->insn_a));
-      if_info->transform_name = "noce_try_store_flag_constants";
+      if_info->transform = ifcvt_transform_noce_try_store_flag_constants;
 
       return TRUE;
     }
@@ -1546,7 +1546,7 @@ noce_try_addcc (struct noce_if_info *if_info)
 
 	      emit_insn_before_setloc (seq, if_info->jump,
 				       INSN_LOCATION (if_info->insn_a));
-	      if_info->transform_name = "noce_try_addcc";
+	      if_info->transform = ifcvt_transform_noce_try_addcc;
 
 	      return TRUE;
 	    }
@@ -1588,7 +1588,7 @@ noce_try_addcc (struct noce_if_info *if_info)
 
 	      emit_insn_before_setloc (seq, if_info->jump,
 				       INSN_LOCATION (if_info->insn_a));
-	      if_info->transform_name = "noce_try_addcc";
+	      if_info->transform = ifcvt_transform_noce_try_addcc;
 	      return TRUE;
 	    }
 	  end_sequence ();
@@ -1639,7 +1639,7 @@ noce_try_store_flag_mask (struct noce_if_info *if_info)
 
 	  emit_insn_before_setloc (seq, if_info->jump,
 				   INSN_LOCATION (if_info->insn_a));
-	  if_info->transform_name = "noce_try_store_flag_mask";
+	  if_info->transform = ifcvt_transform_noce_try_store_flag_mask;
 
 	  return TRUE;
 	}
@@ -1791,7 +1791,7 @@ noce_try_cmove (struct noce_if_info *if_info)
 
 	  emit_insn_before_setloc (seq, if_info->jump,
 				   INSN_LOCATION (if_info->insn_a));
-	  if_info->transform_name = "noce_try_cmove";
+	  if_info->transform = ifcvt_transform_noce_try_cmove;
 
 	  return TRUE;
 	}
@@ -1844,7 +1844,7 @@ noce_try_cmove (struct noce_if_info *if_info)
 
 	      emit_insn_before_setloc (seq, if_info->jump,
 				   INSN_LOCATION (if_info->insn_a));
-	      if_info->transform_name = "noce_try_cmove";
+	      if_info->transform = ifcvt_transform_noce_try_cmove;
 	      return TRUE;
 	    }
 	  else
@@ -2286,7 +2286,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 
   emit_insn_before_setloc (ifcvt_seq, if_info->jump,
 			   INSN_LOCATION (if_info->insn_a));
-  if_info->transform_name = "noce_try_cmove_arith";
+  if_info->transform = ifcvt_transform_noce_try_cmove_arith;
   return TRUE;
 
  end_seq_and_fail:
@@ -2544,7 +2544,7 @@ noce_try_minmax (struct noce_if_info *if_info)
   if_info->cond = cond;
   if_info->cond_earliest = earliest;
   if_info->rev_cond = NULL_RTX;
-  if_info->transform_name = "noce_try_minmax";
+  if_info->transform = ifcvt_transform_noce_try_minmax;
 
   return TRUE;
 }
@@ -2712,7 +2712,7 @@ noce_try_abs (struct noce_if_info *if_info)
   if_info->cond = cond;
   if_info->cond_earliest = earliest;
   if_info->rev_cond = NULL_RTX;
-  if_info->transform_name = "noce_try_abs";
+  if_info->transform = ifcvt_transform_noce_try_abs;
 
   return TRUE;
 }
@@ -2794,7 +2794,7 @@ noce_try_sign_mask (struct noce_if_info *if_info)
     return FALSE;
 
   emit_insn_before_setloc (seq, if_info->jump, INSN_LOCATION (if_info->insn_a));
-  if_info->transform_name = "noce_try_sign_mask";
+  if_info->transform = ifcvt_transform_noce_try_sign_mask;
 
   return TRUE;
 }
@@ -2904,7 +2904,7 @@ noce_try_bitop (struct noce_if_info *if_info)
       emit_insn_before_setloc (seq, if_info->jump,
 			       INSN_LOCATION (if_info->insn_a));
     }
-  if_info->transform_name = "noce_try_bitop";
+  if_info->transform = ifcvt_transform_noce_try_bitop;
   return TRUE;
 }
 
@@ -3271,16 +3271,17 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
   for (int i = 0; i < count; i++)
     noce_emit_move_insn (targets[i], temporaries[i]);
 
+
   /* Actually emit the sequence if it isn't too expensive.  */
   rtx_insn *seq = get_insns ();
 
-  if_info->transform_name = "noce_convert_multiple_sets";
+  if_info->transform = ifcvt_transform_noce_convert_multiple_sets;
   if_info->created_cmovs = count;
 
   if (!targetm.noce_conversion_profitable_p (seq, if_info))
     {
       end_sequence ();
-      if_info->transform_name = "";
+      if_info->transform = ifcvt_transform_none;
       if_info->created_cmovs = 0;
       return FALSE;
     }
@@ -3428,15 +3429,16 @@ noce_process_if_block (struct noce_if_info *if_info)
     {
       if (noce_convert_multiple_sets (if_info))
 	{
-	  if (dump_file && if_info->transform_name)
+	  if (dump_file && if_info->transform)
 	    fprintf (dump_file, "if-conversion succeeded through %s\n",
-		     if_info->transform_name);
+		ifcvt_get_transform_name (if_info->transform));
 	  return TRUE;
 	}
     }
 
   bool speed_p = optimize_bb_for_speed_p (test_bb);
   unsigned int then_cost = 0, else_cost = 0;
+
   if (!bb_valid_for_noce_process_p (then_bb, cond, &then_cost,
 				    &if_info->then_simple))
     return false;
@@ -3647,9 +3649,9 @@ noce_process_if_block (struct noce_if_info *if_info)
   return FALSE;
 
  success:
-  if (dump_file && if_info->transform_name)
+  if (dump_file && if_info->transform)
     fprintf (dump_file, "if-conversion succeeded through %s\n",
-	     if_info->transform_name);
+	ifcvt_get_transform_name (if_info->transform));
 
   /* If we used a temporary, fix it up now.  */
   if (orig_x != x)
@@ -4091,7 +4093,7 @@ noce_find_if_block (basic_block test_bb, edge then_edge, edge else_edge,
      and jump_insns are always given a cost of 1 by seq_cost, so treat
      both instructions as having cost COSTS_N_INSNS (1).  */
   if_info.original_cost = COSTS_N_INSNS (2);
-  if_info.transform_name = "";
+  if_info.transform = ifcvt_transform_none;
   if_info.created_cmovs = 0;
 
   /* Do the real work.  */
diff --git a/gcc/ifcvt.h b/gcc/ifcvt.h
index 50f40bbd1e5..8b4116668a1 100644
--- a/gcc/ifcvt.h
+++ b/gcc/ifcvt.h
@@ -40,6 +40,66 @@ struct ce_if_block
   int pass;				/* Pass number.  */
 };
 
+enum ifcvt_transform
+{
+  ifcvt_transform_none = 0,
+  ifcvt_transform_noce_try_move,
+  ifcvt_transform_noce_try_ifelse_collapse,
+  ifcvt_transform_noce_try_store_flag,
+  ifcvt_transform_noce_try_inverse_constants,
+  ifcvt_transform_noce_try_store_flag_constants,
+  ifcvt_transform_noce_try_addcc,
+  ifcvt_transform_noce_try_store_flag_mask,
+  ifcvt_transform_noce_try_cmove,
+  ifcvt_transform_noce_try_cmove_arith,
+  ifcvt_transform_noce_try_minmax,
+  ifcvt_transform_noce_try_abs,
+  ifcvt_transform_noce_try_sign_mask,
+  ifcvt_transform_noce_try_bitop,
+  ifcvt_transform_noce_convert_multiple_sets
+};
+
+inline const char *
+ifcvt_get_transform_name (enum ifcvt_transform transform)
+{
+  switch (transform)
+    {
+    case ifcvt_transform_none:
+      return "";
+    case ifcvt_transform_noce_try_move:
+      return "noce_try_move";
+    case ifcvt_transform_noce_try_ifelse_collapse:
+      return "noce_try_ifelse_collapse";
+    case ifcvt_transform_noce_try_store_flag:
+      return "noce_try_store_flag";
+    case ifcvt_transform_noce_try_inverse_constants:
+      return "noce_try_inverse_constants";
+    case ifcvt_transform_noce_try_store_flag_constants:
+      return "noce_try_store_flag_constants";
+    case ifcvt_transform_noce_try_addcc:
+      return "noce_try_addcc";
+    case ifcvt_transform_noce_try_store_flag_mask:
+      return "noce_try_store_flag_mask";
+    case ifcvt_transform_noce_try_cmove:
+      return "noce_try_cmove";
+    case ifcvt_transform_noce_try_cmove_arith:
+      return "noce_try_cmove_arith";
+    case ifcvt_transform_noce_try_minmax:
+      return "noce_try_minmax";
+    case ifcvt_transform_noce_try_abs:
+      return "noce_try_abs";
+    case ifcvt_transform_noce_try_sign_mask:
+      return "noce_try_sign_mask";
+    case ifcvt_transform_noce_try_bitop:
+      return "noce_try_bitop";
+    case ifcvt_transform_noce_convert_multiple_sets:
+      return "noce_convert_multiple_sets";
+    default:
+      return "unknown transform";
+    }
+}
+
+
 /* Used by noce_process_if_block to communicate with its subroutines.
 
    The subroutines know that A and B may be evaluated freely.  They
@@ -105,9 +165,10 @@ struct noce_if_info
      generate to replace this branch.  */
   unsigned int max_seq_cost;
 
-  /* The name of the noce transform that succeeded in if-converting
-     this structure.  Used for debugging.  */
-  const char *transform_name;
+  /* The noce transform that succeeded in if-converting this structure.
+     Used for letting the backend determine which transform succeeded
+     and for debugging output.  */
+  enum ifcvt_transform transform;
 
   /* The number of created conditional moves in case we convert multiple
      sets.  */
-- 
2.17.0