diff mbox series

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

Message ID 20181114130752.5057-6-rdapp@linux.ibm.com
State New
Headers show
Series If conversion with multiple sets. | expand

Commit Message

Robin Dapp Nov. 14, 2018, 1:07 p.m. UTC 
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(-)

Comments

Jeff Law Nov. 14, 2018, 9:41 p.m. UTC | #1 
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
Robin Dapp Nov. 15, 2018, 11:38 a.m. UTC | #2 
> 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
Jeff Law Nov. 21, 2018, 12:21 a.m. UTC | #3 
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
diff mbox series

Patch

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);
+
 
 /* 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