public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: <apinski@marvell.com>
To: <gcc-patches@gcc.gnu.org>
Cc: Andrew Pinski <apinski@marvell.com>
Subject: [PATCH] Fix 101256: Wrong code due to range incorrect from PHI-OPT
Date: Mon, 5 Jul 2021 18:13:23 -0700	[thread overview]
Message-ID: <1625534003-6730-1-git-send-email-apinski@marvell.com> (raw)

From: Andrew Pinski <apinski@marvell.com>

So the problem here is that replace_phi_edge_with_variable
will copy range information to a already (not newly) defined
ssa name.  This causes wrong code later on.
This fixes the problem by require the new ssa name to
be defined in the same bb as the conditional that is
about to be deleted.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

Changes from v1:
* this is a simplification of what was trying to be done before.

gcc/ChangeLog:

	PR tree-optimization/101256
	* dbgcnt.def (phiopt_edge_range): New counter.
	* tree-ssa-phiopt.c (replace_phi_edge_with_variable):

gcc/testsuite/ChangeLog:

	PR tree-optimization/101256
	* g++.dg/torture/pr101256.C: New test.
---
 gcc/dbgcnt.def                          |  1 +
 gcc/testsuite/g++.dg/torture/pr101256.C | 28 +++++++++++++++++++++++++
 gcc/tree-ssa-phiopt.c                   | 16 ++++++++------
 3 files changed, 39 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/torture/pr101256.C

diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
index 93e7b4fd30e..2345899ba68 100644
--- a/gcc/dbgcnt.def
+++ b/gcc/dbgcnt.def
@@ -183,6 +183,7 @@ DEBUG_COUNTER (lim)
 DEBUG_COUNTER (local_alloc_for_sched)
 DEBUG_COUNTER (match)
 DEBUG_COUNTER (merged_ipa_icf)
+DEBUG_COUNTER (phiopt_edge_range)
 DEBUG_COUNTER (postreload_cse)
 DEBUG_COUNTER (pre)
 DEBUG_COUNTER (pre_insn)
diff --git a/gcc/testsuite/g++.dg/torture/pr101256.C b/gcc/testsuite/g++.dg/torture/pr101256.C
new file mode 100644
index 00000000000..973a8b4caf3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr101256.C
@@ -0,0 +1,28 @@
+// { dg-do run }
+
+template<class T> 
+const T& max(const T& a, const T& b)
+{
+    return (a < b) ? b : a;
+}
+
+signed char var_5 = -128;
+unsigned int var_11 = 2144479212U;
+unsigned long long int arr [22];
+
+void
+__attribute__((noipa))
+test(signed char var_5, unsigned var_11) {
+  for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
+    arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
+}
+
+int main() {
+  for (int i_0 = 0; i_0 < 22; ++i_0) 
+      arr [i_0] = 11834725929543695741ULL;
+
+  test(var_5, var_11);
+  if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index ab63bf699e3..8b60ee81082 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -51,6 +51,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "internal-fn.h"
 #include "gimple-range.h"
 #include "gimple-match.h"
+#include "dbgcnt.h"
 
 static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
 static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
@@ -390,7 +391,7 @@ replace_phi_edge_with_variable (basic_block cond_block,
   gimple_stmt_iterator gsi;
   tree phi_result = PHI_RESULT (phi);
 
-  /* Duplicate range info if we're the only things setting the target PHI.
+  /* Duplicate range info if they are the only things setting the target PHI.
      This is needed as later on, the new_tree will be replacing
      The assignement of the PHI.
      For an example:
@@ -398,19 +399,22 @@ replace_phi_edge_with_variable (basic_block cond_block,
      _4 = min<a_1, 255>
      goto bb2
 
-     range<-INF,255>
+     # RANGE [-INF, 255]
      a_3 = PHI<_4(1)>
      bb3:
 
      use(a_3)
-     And _4 gets prograted into the use of a_3 and losing the range info.
-     This can't be done for more than 2 incoming edges as the progration
-     won't happen.  */
+     And _4 gets propagated into the use of a_3 and losing the range info.
+     This can't be done for more than 2 incoming edges as the propagation
+     won't happen.
+     The new_tree needs to be defined in the same basic block as the conditional.  */
   if (TREE_CODE (new_tree) == SSA_NAME
       && EDGE_COUNT (gimple_bb (phi)->preds) == 2
       && INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
       && !SSA_NAME_RANGE_INFO (new_tree)
-      && SSA_NAME_RANGE_INFO (phi_result))
+      && SSA_NAME_RANGE_INFO (phi_result)
+      && gimple_bb (SSA_NAME_DEF_STMT (new_tree)) == cond_block
+      && dbg_cnt (phiopt_edge_range))
     duplicate_ssa_name_range_info (new_tree,
 				   SSA_NAME_RANGE_TYPE (phi_result),
 				   SSA_NAME_RANGE_INFO (phi_result));
-- 
2.27.0


             reply	other threads:[~2021-07-06  1:13 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-06  1:13 apinski [this message]
2021-07-06  7:11 ` Richard Biener

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=1625534003-6730-1-git-send-email-apinski@marvell.com \
    --to=apinski@marvell.com \
    --cc=gcc-patches@gcc.gnu.org \
    /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).