Patchwork [PR,debug/45673] fix more MEM_REF -fcompare-debug lossage

login
register
mail settings
Submitter Alexandre Oliva
Date Oct. 1, 2010, 10:08 p.m.
Message ID <or4od54ld2.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/66525/
State New
Headers show

Comments

Alexandre Oliva - Oct. 1, 2010, 10:08 p.m.
Two MEM_REFs can only be considered equivalent, to avoid dumping noise,
if the first operand of both has the same type.  If we don't compare
them, it is possible that two MEM_REFs that convert to the same target
pointer type end up being considered equivalent, although one of them
requires a conversion (that will be dumped) and the other doesn't.

Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?
Richard Guenther - Oct. 2, 2010, 12:49 p.m.
On Sat, Oct 2, 2010 at 12:08 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> Two MEM_REFs can only be considered equivalent, to avoid dumping noise,
> if the first operand of both has the same type.  If we don't compare
> them, it is possible that two MEM_REFs that convert to the same target
> pointer type end up being considered equivalent, although one of them
> requires a conversion (that will be dumped) and the other doesn't.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?

That doesn't make sense.  The only case I can see where it would make a
difference is for an INTEGER_CST operand zero.  And MEM-REFs
_explicitly_ are supposed to ignore the type of the first operand.

I think you are trying to fix things at the wrong place just to make
the dumps you compare equal.  So, what is the testcase in question
that your patch would fix?

Thanks,
Richard.

>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>
>
Alexandre Oliva - Oct. 5, 2010, 4:41 a.m.
On Oct  2, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> MEM-REFs _explicitly_ are supposed to ignore the type of the first
> operand.

And indeed we do ignore it, except in dumps, in which we look at them to
decide whether to print an explicit conversion, and this is where
differences kick in.

> So, what is the testcase in question that your patch would fix?

The one in PR 45673.  Should we implement #c2?
Richard Guenther - Oct. 5, 2010, 9:46 a.m.
On Tue, Oct 5, 2010 at 6:41 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Oct  2, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> MEM-REFs _explicitly_ are supposed to ignore the type of the first
>> operand.
>
> And indeed we do ignore it, except in dumps, in which we look at them to
> decide whether to print an explicit conversion, and this is where
> differences kick in.

The question is why the types differ -g vs. -g0 - I think they should not.

>> So, what is the testcase in question that your patch would fix?
>
> The one in PR 45673.  Should we implement #c2?

I obviously chickened out originally because of the vast amount of testcases
to touch ... and also because of diagnostic messages from late optimization
passes.

What we could do is to always print the MEM variant for INTEGER_CST
operand zero.  That would make sense anyway as you have no way
of guessing the type of operand one by looking at operand zero.
A patch to do this is pre-approved if it passes bootstrap & regtest.

Thanks,
Richard.

> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>
Alexandre Oliva - Oct. 6, 2010, 4:51 a.m.
On Oct  5, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Tue, Oct 5, 2010 at 6:41 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> On Oct  2, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>> 
>>> MEM-REFs _explicitly_ are supposed to ignore the type of the first
>>> operand.
>> 
>> And indeed we do ignore it, except in dumps, in which we look at them to
>> decide whether to print an explicit conversion, and this is where
>> differences kick in.

> The question is why the types differ -g vs. -g0 - I think they should not.

They differ because we consider them the same, and thus mem attr caching
will happily reuse either one for the other.  Debug insns just change
which one gets the shared slot first.

The proposed patch just ensures we don't share the slot, so that
MEM_ATTRs won't vary like this.

> What we could do is to always print the MEM variant for INTEGER_CST
> operand zero.  That would make sense anyway as you have no way
> of guessing the type of operand one by looking at operand zero.
> A patch to do this is pre-approved if it passes bootstrap & regtest.

'k
Jakub Jelinek - Oct. 6, 2010, 7:18 a.m.
On Wed, Oct 06, 2010 at 01:51:28AM -0300, Alexandre Oliva wrote:
> On Oct  5, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
> 
> > On Tue, Oct 5, 2010 at 6:41 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> >> On Oct  2, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
> >> 
> >>> MEM-REFs _explicitly_ are supposed to ignore the type of the first
> >>> operand.
> >> 
> >> And indeed we do ignore it, except in dumps, in which we look at them to
> >> decide whether to print an explicit conversion, and this is where
> >> differences kick in.
> 
> > The question is why the types differ -g vs. -g0 - I think they should not.
> 
> They differ because we consider them the same, and thus mem attr caching
> will happily reuse either one for the other.  Debug insns just change
> which one gets the shared slot first.
> 
> The proposed patch just ensures we don't share the slot, so that
> MEM_ATTRs won't vary like this.

I think it would be better to avoid using operand_equal_p to test whether
to share MEM_ATTRs slot.  Best would be to gather statistics about
how often we share because MEM_EXPR' == MEM_EXPR'', how often the only
differences are in just caused by unshare_expr and no other changes
and how often we accept something differing more than that.
I hope just writing a comparison function that would require operand
equality except for IS_EXPR_CODE_CLASS and for expressions require
everything to be the same (well, perhaps for constants do operand_equal_p),
but the operands which would be checked the same way recursively, would
share most of what we share and would ensure that we print always the same.
Remember operand_equal_p does swap commutative operands etc. for comparison.

	Jakub
Richard Guenther - Oct. 6, 2010, 10:10 a.m.
On Wed, Oct 6, 2010 at 9:18 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Oct 06, 2010 at 01:51:28AM -0300, Alexandre Oliva wrote:
>> On Oct  5, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>>
>> > On Tue, Oct 5, 2010 at 6:41 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>> >> On Oct  2, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>> >>
>> >>> MEM-REFs _explicitly_ are supposed to ignore the type of the first
>> >>> operand.
>> >>
>> >> And indeed we do ignore it, except in dumps, in which we look at them to
>> >> decide whether to print an explicit conversion, and this is where
>> >> differences kick in.
>>
>> > The question is why the types differ -g vs. -g0 - I think they should not.
>>
>> They differ because we consider them the same, and thus mem attr caching
>> will happily reuse either one for the other.  Debug insns just change
>> which one gets the shared slot first.
>>
>> The proposed patch just ensures we don't share the slot, so that
>> MEM_ATTRs won't vary like this.
>
> I think it would be better to avoid using operand_equal_p to test whether
> to share MEM_ATTRs slot.  Best would be to gather statistics about
> how often we share because MEM_EXPR' == MEM_EXPR'', how often the only
> differences are in just caused by unshare_expr and no other changes
> and how often we accept something differing more than that.
> I hope just writing a comparison function that would require operand
> equality except for IS_EXPR_CODE_CLASS and for expressions require
> everything to be the same (well, perhaps for constants do operand_equal_p),
> but the operands which would be checked the same way recursively, would
> share most of what we share and would ensure that we print always the same.
> Remember operand_equal_p does swap commutative operands etc. for comparison.

Indeed.  I think MEM_ATTR sharing wants a lexical compare, not a
semantic one as operand_equal_p provides.

Considering to remove MEM_ATTR sharing alltogether also makes
sense given that no longer pruning MEM_EXPR that aggressively
very likely resulted in way less sharing than we had before.

Richard.

>        Jakub
>

Patch

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/45673
	PR debug/45604
	PR debug/45419
	PR debug/45408
	* fold-const.c (operand_equal_p): Compare both pointer types
	of MEM_REFs.
	* tree.c (iterative_hash_expr): Hash both of them too.

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c.orig	2010-09-21 05:42:27.671516671 -0300
+++ gcc/fold-const.c	2010-09-21 05:45:03.200676171 -0300
@@ -2600,6 +2600,8 @@  operand_equal_p (const_tree arg0, const_
 		       && TYPE_SIZE (TREE_TYPE (arg1))
 		       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
 					   TYPE_SIZE (TREE_TYPE (arg1)), flags)))
+		  && (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg0, 0)))
+		      == TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg1, 0))))
 		  && (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg0, 1)))
 		      == TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg1, 1))))
 		  && OP_SAME (0) && OP_SAME (1));
Index: gcc/tree.c
===================================================================
--- gcc/tree.c.orig	2010-09-21 05:42:27.137424998 -0300
+++ gcc/tree.c	2010-09-21 05:44:20.498430669 -0300
@@ -6791,20 +6791,21 @@  iterative_hash_expr (const_tree t, hashv
 	return val;
       }
     case MEM_REF:
-      {
-	/* The type of the second operand is relevant, except for
-	   its top-level qualifiers.  */
-	tree type = TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (t, 1)));
-
-	val = iterative_hash_object (TYPE_HASH (type), val);
-
-	/* We could use the standard hash computation from this point
-	   on.  */
-	val = iterative_hash_object (code, val);
-	val = iterative_hash_expr (TREE_OPERAND (t, 1), val);
-	val = iterative_hash_expr (TREE_OPERAND (t, 0), val);
-	return val;
-      }
+      for (i = 0; i <= 1; i++)
+	{
+	  /* The types of the operands are relevant, except for their
+	     top-level qualifiers.  */
+	  tree type = TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (t, i)));
+
+	  val = iterative_hash_object (TYPE_HASH (type), val);
+	}
+
+      /* We could use the standard hash computation from this point
+	 on.  */
+      val = iterative_hash_object (code, val);
+      val = iterative_hash_expr (TREE_OPERAND (t, 1), val);
+      val = iterative_hash_expr (TREE_OPERAND (t, 0), val);
+      return val;
     case FUNCTION_DECL:
       /* When referring to a built-in FUNCTION_DECL, use the __builtin__ form.
 	 Otherwise nodes that compare equal according to operand_equal_p might