public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Robin Dapp <rdapp@linux.ibm.com>
To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com
Subject: [PATCH 1/7] ifcvt: Check if cmovs are needed.
Date: Fri, 25 Jun 2021 18:08:59 +0200	[thread overview]
Message-ID: <20210625160905.23786-2-rdapp@linux.ibm.com> (raw)
In-Reply-To: <20210625160905.23786-1-rdapp@linux.ibm.com>

When if-converting multiple SETs and we encounter a swap-style idiom

  if (a > b)
    {
      tmp = c;   // [1]
      c = d;
      d = tmp;
    }

ifcvt should not generate a conditional move for the instruction at
[1].

In order to achieve that, this patch goes through all relevant SETs
and marks the relevant instructions.  This helps to evaluate costs.

On top, only generate temporaries if the current cmov is going to
overwrite one of the comparands of the initial compare.
---
 gcc/ifcvt.c | 104 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 87 insertions(+), 17 deletions(-)

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 017944f4f79..eef6490626a 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -98,6 +98,7 @@ 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_cmovs (basic_block, hash_map<rtx, bool> *);
 \f
 /* Count the number of non-jump active insns in BB.  */
 
@@ -3203,6 +3204,10 @@ noce_convert_multiple_sets (struct noce_if_info *if_info)
   auto_vec<rtx_insn *> unmodified_insns;
   int count = 0;
 
+  hash_map<rtx, bool> need_cmovs;
+
+  check_need_cmovs (then_bb, &need_cmovs);
+
   FOR_BB_INSNS (then_bb, insn)
     {
       /* Skip over non-insns.  */
@@ -3213,26 +3218,38 @@ 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 temp;
       rtx new_val = SET_SRC (set);
       rtx old_val = 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.  */
-      for (int i = count - 1; i >= 0; --i)
-	if (reg_overlap_mentioned_p (new_val, targets[i]))
-	  {
-	    /* Catch a "swap" style idiom.  */
-	    if (find_reg_note (insn, REG_DEAD, new_val) != NULL_RTX)
-	      /* The write to targets[i] is only live until the read
-		 here.  As the condition codes match, we can propagate
-		 the set to here.  */
-	      new_val = SET_SRC (single_set (unmodified_insns[i]));
-	    else
-	      new_val = temporaries[i];
-	    break;
-	  }
+      /* As we are transforming
+	 if (x > y)
+	   a = b;
+	   c = d;
+	 into
+	   a = (x > y) ...
+	   c = (x > y) ...
+
+	 we potentially check x > y before every set here.
+	 (Even though might be removed by subsequent passes.)
+	 We cannot transform
+	   if (x > y)
+	     x = y;
+	     ...
+	 into
+	   x = (x > y) ...
+	   ...
+	 since this would invalidate x.  Therefore we introduce a temporary
+	 every time we are about to overwrite a variable used in the
+	 check.  Costing of a sequence with these is going to be inaccurate.  */
+      if (reg_overlap_mentioned_p (target, cond))
+	temp = gen_reg_rtx (GET_MODE (target));
+      else
+	temp = target;
+
+      bool need_cmov = true;
+      if (need_cmovs.get (insn))
+	need_cmov = false;
 
       /* If we had a non-canonical conditional jump (i.e. one where
 	 the fallthrough is to the "else" case) we need to reverse
@@ -3808,6 +3825,59 @@ check_cond_move_block (basic_block bb,
   return TRUE;
 }
 
+/* Find local swap-style idioms in BB and mark the first insn (1)
+   that is only a temporary as not needing a conditional move as
+   it is going to be dead afterwards anyway.
+
+     (1) int tmp = a;
+	 a = b;
+	 b = tmp;
+
+	 ifcvt
+	 -->
+
+	 load tmp,a
+	 cmov a,b
+	 cmov b,tmp   */
+
+static void
+check_need_cmovs (basic_block bb, hash_map<rtx, bool> *need_cmov)
+{
+  rtx_insn *insn;
+  int count = 0;
+  auto_vec<rtx> insns;
+  auto_vec<rtx> dests;
+
+  FOR_BB_INSNS (bb, insn)
+    {
+      rtx set, src, dest;
+
+      if (!active_insn_p (insn))
+	continue;
+
+      set = single_set (insn);
+      if (set == NULL_RTX)
+	continue;
+
+      src = SET_SRC (set);
+      dest = SET_DEST (set);
+
+      for (int i = count - 1; i >= 0; --i)
+	{
+	  if (reg_overlap_mentioned_p (src, dests[i])
+	      && find_reg_note (insn, REG_DEAD, src) != NULL_RTX)
+	    {
+	      need_cmov->put (insns[i], false);
+	    }
+	}
+
+      insns.safe_push (insn);
+      dests.safe_push (dest);
+
+      count++;
+    }
+}
+
 /* 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.31.1


  reply	other threads:[~2021-06-25 16:09 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25 16:08 [PATCH 0/7] ifcvt: Convert multiple Robin Dapp
2021-06-25 16:08 ` Robin Dapp [this message]
2021-07-15 20:10   ` [PATCH 1/7] ifcvt: Check if cmovs are needed Richard Sandiford
2021-07-22 12:06     ` Robin Dapp
2021-07-26 19:08       ` Richard Sandiford
2021-09-15  8:39         ` Robin Dapp
2021-10-14  8:45           ` Richard Sandiford
2021-10-14 14:20             ` Robin Dapp
2021-10-14 14:32               ` Richard Sandiford
2021-10-18 11:40                 ` Robin Dapp
2021-11-03  8:55                   ` Robin Dapp
2021-11-05 15:33                   ` Richard Sandiford
2021-11-12 13:00                     ` Robin Dapp
2021-11-30 16:36                       ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 2/7] ifcvt: Allow constants for noce_convert_multiple Robin Dapp
2021-07-15 20:25   ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 3/7] ifcvt: Improve costs handling " Robin Dapp
2021-07-15 20:42   ` Richard Sandiford
2021-07-22 12:07     ` Robin Dapp
2021-07-26 19:10       ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 4/7] ifcvt/optabs: Allow using a CC comparison for emit_conditional_move Robin Dapp
2021-07-15 20:54   ` Richard Sandiford
2021-07-22 12:07     ` Robin Dapp
2021-07-26 19:31       ` Richard Sandiford
2021-07-27 20:49         ` Robin Dapp
2021-08-06 12:14           ` Richard Sandiford
2021-06-25 16:09 ` [PATCH 5/7] ifcvt: Try re-using CC for conditional moves Robin Dapp
2021-07-22 12:12   ` Robin Dapp
2021-06-25 16:09 ` [PATCH 6/7] testsuite/s390: Add tests for noce_convert_multiple Robin Dapp
2021-06-25 16:09 ` [PATCH 7/7] s390: Increase costs for load on condition and change movqicc expander Robin Dapp
2021-07-13 12:42 ` [PATCH 0/7] ifcvt: Convert multiple Robin Dapp

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210625160905.23786-2-rdapp@linux.ibm.com \
    --to=rdapp@linux.ibm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=richard.sandiford@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).