| Message ID | or4od54ld2.fsf@livre.localdomain |
|---|---|
| State | New |
| Headers | show |
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 > >
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?
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 >
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
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
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 >
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