This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 2/6] gimple-ssa-store-merging.c: fix sort_by_bitpos
On Tue, Jul 25, 2017 at 10:34 AM, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi,
>
> On 24/07/17 21:48, Alexander Monakov wrote:
>>
>> On Sat, 22 Jul 2017, Segher Boessenkool wrote:
>>
>>> On Sat, Jul 15, 2017 at 11:47:45PM +0300, Alexander Monakov wrote:
>>>>
>>>> --- a/gcc/gimple-ssa-store-merging.c
>>>> +++ b/gcc/gimple-ssa-store-merging.c
>>>> @@ -516,12 +516,12 @@ sort_by_bitpos (const void *x, const void *y)
>>>> store_immediate_info *const *tmp = (store_immediate_info * const *)
>>>> x;
>>>> store_immediate_info *const *tmp2 = (store_immediate_info * const *)
>>>> y;
>>>> - if ((*tmp)->bitpos <= (*tmp2)->bitpos)
>>>> + if ((*tmp)->bitpos < (*tmp2)->bitpos)
>>>> return -1;
>>>> else if ((*tmp)->bitpos > (*tmp2)->bitpos)
>>>> return 1;
>>>> -
>>>> - gcc_unreachable ();
>>>> + else
>>>> + return 0;
>>>> }
>>>
>>> Does any sort using this comparison function require the sort to be
>>> stable?
>>>
>>> It looks like the <= was simply a typo and should have been <, but
>>> the gcc_unreachable was as intended? (With <= it is trivially
>>> unreachable there).
>>
>> I think it's best if the original author can chime in - adding Kyrill.
>>
>> (to be clear, equal bitpos is possible, this issue causes qsort checker
>> errors)
>
>
> For the uses of this function the order when the bitpos is the same
> does not matter, I just wanted to avoid returning zero to avoid
> perturbations
> due to qsort.
> IMO the right thing to do here to avoid the qsort checker errors is to break
> the tie between
> store_immediate_info objects with equal bitpos by using the sort_by_order
> heuristic
> i.e. something like "return (*tmp)->order - (*tmp2)->order;".
> That should give well-behaved results as the order of two
> store_immediate_info objects
> is guaranteed not to be the same (otherwise something has gone wrong
> elsewhere).
Agreed.
Richard.
> Thanks,
> Kyrill
>
>> Alexander
>
>