[PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Martin Liška-2
Hi.

operand_equal_p can properly handle situation where we have a CONSTRUCTOR
where indices are NULL:

              if (!operand_equal_p (c0->value, c1->value, flags)
                  /* In GIMPLE the indexes can be either NULL or matching i.
                     Double check this so we won't get false
                     positives for GENERIC.  */
                  || (c0->index
                      && (TREE_CODE (c0->index) != INTEGER_CST
                          || compare_tree_int (c0->index, i)))
                  || (c1->index
                      && (TREE_CODE (c1->index) != INTEGER_CST
                          || compare_tree_int (c1->index, i))))
                return false;

but the corresponding hash function always hashes field (which
can be NULL_TREE or equal to ctor index).

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

2019-10-31  Martin Liska  <[hidden email]>

        PR ipa/92304
        * fold-const.c (operand_compare::hash_operand): Fix field
        hashing of CONSTRUCTOR.
---
 gcc/fold-const.c | 3 +++
 1 file changed, 3 insertions(+)



0001-Fix-hash_operand-for-fields-of-a-CONSTRUCTOR.patch (568 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Jeff Law
On 10/31/19 10:01 AM, Martin Liška wrote:

> Hi.
>
> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
> where indices are NULL:
>
>      if (!operand_equal_p (c0->value, c1->value, flags)
>  /* In GIMPLE the indexes can be either NULL or matching i.
>     Double check this so we won't get false
>     positives for GENERIC.  */
>  || (c0->index
>      && (TREE_CODE (c0->index) != INTEGER_CST
>  || compare_tree_int (c0->index, i)))
>  || (c1->index
>      && (TREE_CODE (c1->index) != INTEGER_CST
>  || compare_tree_int (c1->index, i))))
> return false;
>
> but the corresponding hash function always hashes field (which
> can be NULL_TREE or equal to ctor index).
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2019-10-31  Martin Liska  <[hidden email]>
>
> PR ipa/92304
> * fold-const.c (operand_compare::hash_operand): Fix field
> hashing of CONSTRUCTOR.
OK.  One question though, do these routines need to handle
CONSTRUCTOR_NO_CLEARING?

jeff

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Martin Liška-2
On 11/1/19 10:51 PM, Jeff Law wrote:

> On 10/31/19 10:01 AM, Martin Liška wrote:
>> Hi.
>>
>> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
>> where indices are NULL:
>>
>>      if (!operand_equal_p (c0->value, c1->value, flags)
>>  /* In GIMPLE the indexes can be either NULL or matching i.
>>     Double check this so we won't get false
>>     positives for GENERIC.  */
>>  || (c0->index
>>      && (TREE_CODE (c0->index) != INTEGER_CST
>>  || compare_tree_int (c0->index, i)))
>>  || (c1->index
>>      && (TREE_CODE (c1->index) != INTEGER_CST
>>  || compare_tree_int (c1->index, i))))
>> return false;
>>
>> but the corresponding hash function always hashes field (which
>> can be NULL_TREE or equal to ctor index).
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2019-10-31  Martin Liska  <[hidden email]>
>>
>> PR ipa/92304
>> * fold-const.c (operand_compare::hash_operand): Fix field
>> hashing of CONSTRUCTOR.
> OK.  One question though, do these routines need to handle
> CONSTRUCTOR_NO_CLEARING?

Good point, but I bet it's just a flag used in GENERIC, right?

Martin

>
> jeff
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Richard Biener-2
On Mon, Nov 4, 2019 at 10:09 AM Martin Liška <[hidden email]> wrote:

>
> On 11/1/19 10:51 PM, Jeff Law wrote:
> > On 10/31/19 10:01 AM, Martin Liška wrote:
> >> Hi.
> >>
> >> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
> >> where indices are NULL:
> >>
> >>            if (!operand_equal_p (c0->value, c1->value, flags)
> >>                /* In GIMPLE the indexes can be either NULL or matching i.
> >>                   Double check this so we won't get false
> >>                   positives for GENERIC.  */
> >>                || (c0->index
> >>                    && (TREE_CODE (c0->index) != INTEGER_CST
> >>                        || compare_tree_int (c0->index, i)))
> >>                || (c1->index
> >>                    && (TREE_CODE (c1->index) != INTEGER_CST
> >>                        || compare_tree_int (c1->index, i))))
> >>              return false;
> >>
> >> but the corresponding hash function always hashes field (which
> >> can be NULL_TREE or equal to ctor index).
> >>
> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>
> >> Ready to be installed?
> >> Thanks,
> >> Martin
> >>
> >> gcc/ChangeLog:
> >>
> >> 2019-10-31  Martin Liska  <[hidden email]>
> >>
> >>      PR ipa/92304
> >>      * fold-const.c (operand_compare::hash_operand): Fix field
> >>      hashing of CONSTRUCTOR.
> > OK.  One question though, do these routines need to handle
> > CONSTRUCTOR_NO_CLEARING?
>
> Good point, but I bet it's just a flag used in GENERIC, right?

Yes.  It matters for gimplification only.  I don't think we can
optimistically make use of it in operand_equal_p.

Richard.

> Martin
>
> >
> > jeff
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Richard Biener-2
On Mon, Nov 4, 2019 at 2:35 PM Richard Biener
<[hidden email]> wrote:

>
> On Mon, Nov 4, 2019 at 10:09 AM Martin Liška <[hidden email]> wrote:
> >
> > On 11/1/19 10:51 PM, Jeff Law wrote:
> > > On 10/31/19 10:01 AM, Martin Liška wrote:
> > >> Hi.
> > >>
> > >> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
> > >> where indices are NULL:
> > >>
> > >>            if (!operand_equal_p (c0->value, c1->value, flags)
> > >>                /* In GIMPLE the indexes can be either NULL or matching i.
> > >>                   Double check this so we won't get false
> > >>                   positives for GENERIC.  */
> > >>                || (c0->index
> > >>                    && (TREE_CODE (c0->index) != INTEGER_CST
> > >>                        || compare_tree_int (c0->index, i)))
> > >>                || (c1->index
> > >>                    && (TREE_CODE (c1->index) != INTEGER_CST
> > >>                        || compare_tree_int (c1->index, i))))
> > >>              return false;
> > >>
> > >> but the corresponding hash function always hashes field (which
> > >> can be NULL_TREE or equal to ctor index).
> > >>
> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > >>
> > >> Ready to be installed?
> > >> Thanks,
> > >> Martin
> > >>
> > >> gcc/ChangeLog:
> > >>
> > >> 2019-10-31  Martin Liska  <[hidden email]>
> > >>
> > >>      PR ipa/92304
> > >>      * fold-const.c (operand_compare::hash_operand): Fix field
> > >>      hashing of CONSTRUCTOR.
> > > OK.  One question though, do these routines need to handle
> > > CONSTRUCTOR_NO_CLEARING?
> >
> > Good point, but I bet it's just a flag used in GENERIC, right?
>
> Yes.  It matters for gimplification only.  I don't think we can
> optimistically make use of it in operand_equal_p.

OTOH for GENERIC and sth like ICF the flags have to match.

Richard.

> Richard.
>
> > Martin
> >
> > >
> > > jeff
> > >
> >
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Jeff Law
In reply to this post by Richard Biener-2
On 11/4/19 6:35 AM, Richard Biener wrote:

> On Mon, Nov 4, 2019 at 10:09 AM Martin Liška <[hidden email]> wrote:
>>
>> On 11/1/19 10:51 PM, Jeff Law wrote:
>>> On 10/31/19 10:01 AM, Martin Liška wrote:
>>>> Hi.
>>>>
>>>> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
>>>> where indices are NULL:
>>>>
>>>>            if (!operand_equal_p (c0->value, c1->value, flags)
>>>>                /* In GIMPLE the indexes can be either NULL or matching i.
>>>>                   Double check this so we won't get false
>>>>                   positives for GENERIC.  */
>>>>                || (c0->index
>>>>                    && (TREE_CODE (c0->index) != INTEGER_CST
>>>>                        || compare_tree_int (c0->index, i)))
>>>>                || (c1->index
>>>>                    && (TREE_CODE (c1->index) != INTEGER_CST
>>>>                        || compare_tree_int (c1->index, i))))
>>>>              return false;
>>>>
>>>> but the corresponding hash function always hashes field (which
>>>> can be NULL_TREE or equal to ctor index).
>>>>
>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>
>>>> Ready to be installed?
>>>> Thanks,
>>>> Martin
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2019-10-31  Martin Liska  <[hidden email]>
>>>>
>>>>      PR ipa/92304
>>>>      * fold-const.c (operand_compare::hash_operand): Fix field
>>>>      hashing of CONSTRUCTOR.
>>> OK.  One question though, do these routines need to handle
>>> CONSTRUCTOR_NO_CLEARING?
>>
>> Good point, but I bet it's just a flag used in GENERIC, right?
>
> Yes.  It matters for gimplification only.  I don't think we can
> optimistically make use of it in operand_equal_p.
I'm not thinking about optimistically using it, quite the opposite :-)

My worry is that we could potentially consider two CONSTRUCTOR nodes
equal in the hashing and equality functions when they really aren't
equal due to the semantics around CONSTRUCTOR_NO_CLEARING.

jeff

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Jeff Law
In reply to this post by Richard Biener-2
On 11/4/19 6:36 AM, Richard Biener wrote:

> On Mon, Nov 4, 2019 at 2:35 PM Richard Biener
> <[hidden email]> wrote:
>>
>> On Mon, Nov 4, 2019 at 10:09 AM Martin Liška <[hidden email]> wrote:
>>>
>>> On 11/1/19 10:51 PM, Jeff Law wrote:
>>>> On 10/31/19 10:01 AM, Martin Liška wrote:
>>>>> Hi.
>>>>>
>>>>> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
>>>>> where indices are NULL:
>>>>>
>>>>>            if (!operand_equal_p (c0->value, c1->value, flags)
>>>>>                /* In GIMPLE the indexes can be either NULL or matching i.
>>>>>                   Double check this so we won't get false
>>>>>                   positives for GENERIC.  */
>>>>>                || (c0->index
>>>>>                    && (TREE_CODE (c0->index) != INTEGER_CST
>>>>>                        || compare_tree_int (c0->index, i)))
>>>>>                || (c1->index
>>>>>                    && (TREE_CODE (c1->index) != INTEGER_CST
>>>>>                        || compare_tree_int (c1->index, i))))
>>>>>              return false;
>>>>>
>>>>> but the corresponding hash function always hashes field (which
>>>>> can be NULL_TREE or equal to ctor index).
>>>>>
>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>
>>>>> Ready to be installed?
>>>>> Thanks,
>>>>> Martin
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> 2019-10-31  Martin Liska  <[hidden email]>
>>>>>
>>>>>      PR ipa/92304
>>>>>      * fold-const.c (operand_compare::hash_operand): Fix field
>>>>>      hashing of CONSTRUCTOR.
>>>> OK.  One question though, do these routines need to handle
>>>> CONSTRUCTOR_NO_CLEARING?
>>>
>>> Good point, but I bet it's just a flag used in GENERIC, right?
>>
>> Yes.  It matters for gimplification only.  I don't think we can
>> optimistically make use of it in operand_equal_p.
>
> OTOH for GENERIC and sth like ICF the flags have to match.
Precisely my concern.  I'm not immediately aware of any case where it
matters, but it'd be nice to future proof this if we can.

jeff

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Martin Liška-2
On 11/4/19 4:24 PM, Jeff Law wrote:

> On 11/4/19 6:36 AM, Richard Biener wrote:
>> On Mon, Nov 4, 2019 at 2:35 PM Richard Biener
>> <[hidden email]> wrote:
>>>
>>> On Mon, Nov 4, 2019 at 10:09 AM Martin Liška <[hidden email]> wrote:
>>>>
>>>> On 11/1/19 10:51 PM, Jeff Law wrote:
>>>>> On 10/31/19 10:01 AM, Martin Liška wrote:
>>>>>> Hi.
>>>>>>
>>>>>> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
>>>>>> where indices are NULL:
>>>>>>
>>>>>>             if (!operand_equal_p (c0->value, c1->value, flags)
>>>>>>                 /* In GIMPLE the indexes can be either NULL or matching i.
>>>>>>                    Double check this so we won't get false
>>>>>>                    positives for GENERIC.  */
>>>>>>                 || (c0->index
>>>>>>                     && (TREE_CODE (c0->index) != INTEGER_CST
>>>>>>                         || compare_tree_int (c0->index, i)))
>>>>>>                 || (c1->index
>>>>>>                     && (TREE_CODE (c1->index) != INTEGER_CST
>>>>>>                         || compare_tree_int (c1->index, i))))
>>>>>>               return false;
>>>>>>
>>>>>> but the corresponding hash function always hashes field (which
>>>>>> can be NULL_TREE or equal to ctor index).
>>>>>>
>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>
>>>>>> Ready to be installed?
>>>>>> Thanks,
>>>>>> Martin
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 2019-10-31  Martin Liska  <[hidden email]>
>>>>>>
>>>>>>       PR ipa/92304
>>>>>>       * fold-const.c (operand_compare::hash_operand): Fix field
>>>>>>       hashing of CONSTRUCTOR.
>>>>> OK.  One question though, do these routines need to handle
>>>>> CONSTRUCTOR_NO_CLEARING?
>>>>
>>>> Good point, but I bet it's just a flag used in GENERIC, right?
>>>
>>> Yes.  It matters for gimplification only.  I don't think we can
>>> optimistically make use of it in operand_equal_p.
>>
>> OTOH for GENERIC and sth like ICF the flags have to match.
> Precisely my concern.  I'm not immediately aware of any case where it
> matters, but it'd be nice to future proof this if we can.
>
> jeff
>
Sure, I've got the following tested patch.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

0001-Add-CONSTRUCTOR_NO_CLEARING-to-operand_equal_p.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix hash_operand for fields of a CONSTRUCTOR.

Jeff Law
On 11/5/19 1:35 AM, Martin Liška wrote:

> On 11/4/19 4:24 PM, Jeff Law wrote:
>> On 11/4/19 6:36 AM, Richard Biener wrote:
>>> On Mon, Nov 4, 2019 at 2:35 PM Richard Biener
>>> <[hidden email]> wrote:
>>>>
>>>> On Mon, Nov 4, 2019 at 10:09 AM Martin Liška <[hidden email]> wrote:
>>>>>
>>>>> On 11/1/19 10:51 PM, Jeff Law wrote:
>>>>>> On 10/31/19 10:01 AM, Martin Liška wrote:
>>>>>>> Hi.
>>>>>>>
>>>>>>> operand_equal_p can properly handle situation where we have a CONSTRUCTOR
>>>>>>>
>>>>>>> where indices are NULL:
>>>>>>>
>>>>>>>             if (!operand_equal_p (c0->value, c1->value, flags)
>>>>>>>                 /* In GIMPLE the indexes can be either NULL or matching i.
>>>>>>>
>>>>>>>                    Double check this so we won't get false
>>>>>>>                    positives for GENERIC.  */
>>>>>>>                 || (c0->index
>>>>>>>                     && (TREE_CODE (c0->index) != INTEGER_CST
>>>>>>>                         || compare_tree_int (c0->index, i)))
>>>>>>>                 || (c1->index
>>>>>>>                     && (TREE_CODE (c1->index) != INTEGER_CST
>>>>>>>                         || compare_tree_int (c1->index, i))))
>>>>>>>               return false;
>>>>>>>
>>>>>>> but the corresponding hash function always hashes field (which
>>>>>>> can be NULL_TREE or equal to ctor index).
>>>>>>>
>>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>>
>>>>>>>
>>>>>>> Ready to be installed?
>>>>>>> Thanks,
>>>>>>> Martin
>>>>>>>
>>>>>>> gcc/ChangeLog:
>>>>>>>
>>>>>>> 2019-10-31  Martin Liska  <[hidden email]>
>>>>>>>
>>>>>>>       PR ipa/92304
>>>>>>>       * fold-const.c (operand_compare::hash_operand): Fix field
>>>>>>>       hashing of CONSTRUCTOR.
>>>>>> OK.  One question though, do these routines need to handle
>>>>>> CONSTRUCTOR_NO_CLEARING?
>>>>>
>>>>> Good point, but I bet it's just a flag used in GENERIC, right?
>>>>
>>>> Yes.  It matters for gimplification only.  I don't think we can
>>>> optimistically make use of it in operand_equal_p.
>>>
>>> OTOH for GENERIC and sth like ICF the flags have to match.
>> Precisely my concern.  I'm not immediately aware of any case where it
>> matters, but it'd be nice to future proof this if we can.
>>
>> jeff
>>
>
> Sure, I've got the following tested patch.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> 0001-Add-CONSTRUCTOR_NO_CLEARING-to-operand_equal_p.patch
>
> From 2302c15cb2568bc71b4b7bc3abbfd66aafc7c06c Mon Sep 17 00:00:00 2001
> From: Martin Liska <[hidden email]>
> Date: Mon, 4 Nov 2019 15:39:40 +0100
> Subject: [PATCH] Add CONSTRUCTOR_NO_CLEARING to operand_equal_p.
>
> gcc/ChangeLog:
>
> 2019-11-04  Martin Liska  <[hidden email]>
>
> * fold-const.c (operand_compare::operand_equal_p): Add comparison
> of CONSTRUCTOR_NO_CLEARING.
> (operand_compare::hash_operand): Likewise.
OK
jeff