public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Richard Biener <rguenther@suse.de>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
Date: Tue, 04 Jul 2017 11:32:00 -0000	[thread overview]
Message-ID: <20170704113247.GI2123@tucnak> (raw)

Hi!

The compare_assert_loc added recently to sort assert exprs that could
operand_equal_p the expressions/values in there unfortunately broke
-fcompare-debug.  The problem is that DECL_UIDs don't have to be the same
between -g and -g0, and thus what iterative_hash_expr returns might not be
the same.  For the removal of duplicate assert exprs or moving assert expr
to the dominator if present on all edges this doesn't matter, because
all we care about there are the adjacent vector entries and code generation
is not affected by the traversal order, but when we actually
process_assert_insertions_for afterwards, the order matters a lot for
code generation (different SSA_NAME_VERSION on the ASSERT_EXPR lhs will
mean different order of release_ssa_name afterwards and might result
in different SSA_NAME versions created later on in other passes and that
in many cases affects code generation.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?  No testcase, as the reduced testcase doesn't reproduce it (at least
for me) and the original is way too large).

2017-07-04  Jakub Jelinek  <jakub@redhat.com>

	PR debug/81278
	* tree-vrp.c (compare_assert_loc): Only test if a->e is NULL,
	!a->e == !b->e has been verified already.  Use e == NULL or
	e != NULL instead of e or ! e tests.
	(compare_assert_loc_stable): New function.
	(process_assert_insertions): Sort using compare_assert_loc_stable
	before calling process_assert_insertions_for in a loop.  Use break
	instead of continue once seen NULL pointer.

--- gcc/tree-vrp.c.jj	2017-07-03 19:03:23.817824263 +0200
+++ gcc/tree-vrp.c	2017-07-04 10:30:36.403204757 +0200
@@ -6400,13 +6400,13 @@ compare_assert_loc (const void *pa, cons
 {
   assert_locus * const a = *(assert_locus * const *)pa;
   assert_locus * const b = *(assert_locus * const *)pb;
-  if (! a->e && b->e)
+  if (a->e == NULL && b->e != NULL)
     return 1;
-  else if (a->e && ! b->e)
+  else if (a->e != NULL && b->e == NULL)
     return -1;
 
   /* Sort after destination index.  */
-  if (! a->e && ! b->e)
+  if (a->e == NULL)
     ;
   else if (a->e->dest->index > b->e->dest->index)
     return 1;
@@ -6423,12 +6423,53 @@ compare_assert_loc (const void *pa, cons
   hashval_t ha = iterative_hash_expr (a->expr, iterative_hash_expr (a->val, 0));
   hashval_t hb = iterative_hash_expr (b->expr, iterative_hash_expr (b->val, 0));
   if (ha == hb)
-    return (a->e && b->e
+    return (a->e != NULL
 	    ? a->e->src->index - b->e->src->index
 	    : a->bb->index - b->bb->index);
   return ha - hb;
 }
 
+/* Qsort helper for sorting assert locations.  Like the above, but
+   don't use expression hashes for the sorting to make the sorting
+   stable for -fcompare-debug.  Some assert_locus pointers could
+   be NULL, sort those last.  */
+
+static int
+compare_assert_loc_stable (const void *pa, const void *pb)
+{
+  assert_locus * const a = *(assert_locus * const *)pa;
+  assert_locus * const b = *(assert_locus * const *)pb;
+
+  if (a == NULL)
+    return b != NULL;
+  else if (b == NULL)
+    return -1;
+
+  if (a->e == NULL && b->e != NULL)
+    return 1;
+  else if (a->e != NULL && b->e == NULL)
+    return -1;
+
+  /* Sort after destination index.  */
+  if (a->e == NULL)
+    ;
+  else if (a->e->dest->index > b->e->dest->index)
+    return 1;
+  else if (a->e->dest->index < b->e->dest->index)
+    return -1;
+
+  /* Sort after comp_code.  */
+  if (a->comp_code > b->comp_code)
+    return 1;
+  else if (a->comp_code < b->comp_code)
+    return -1;
+
+  /* Break the tie using source/bb index.  */
+  return (a->e != NULL
+	  ? a->e->src->index - b->e->src->index
+	  : a->bb->index - b->bb->index);
+}
+
 /* Process all the insertions registered for every name N_i registered
    in NEED_ASSERT_FOR.  The list of assertions to be inserted are
    found in ASSERTS_FOR[i].  */
@@ -6506,11 +6547,12 @@ process_assert_insertions (void)
 	    }
 	}
 
+      asserts.qsort (compare_assert_loc_stable);
       for (unsigned j = 0; j < asserts.length (); ++j)
 	{
 	  loc = asserts[j];
 	  if (! loc)
-	    continue;
+	    break;
 	  update_edges_p |= process_assert_insertions_for (ssa_name (i), loc);
 	  num_asserts++;
 	  free (loc);

	Jakub

             reply	other threads:[~2017-07-04 11:32 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-04 11:32 Jakub Jelinek [this message]
2017-07-04 11:46 ` Richard Biener
2017-07-04 11:50   ` Jakub Jelinek
2017-07-04 12:00     ` Richard Biener
2017-07-04 12:42       ` Jakub Jelinek
2017-07-04 14:06         ` Richard Biener
2017-07-04 15:07         ` Jeff Law

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=20170704113247.GI2123@tucnak \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=rguenther@suse.de \
    /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).