un-#ifdef GATHER_STATISTICS

On Tue, Jul 24, 2012 at 4:37 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Tue, Jul 24, 2012 at 3:08 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> This patch (r189803) regressed a bunch of tests on x86_64 [1], [2].
>>
>> [1] http://gcc.gnu.org/ml/gcc-testresults/2012-07/msg02066.html
>> [2] http://gcc.gnu.org/ml/gcc-regression/2012-07/msg00177.html
>
> These are all selective-scheduler test cases. It looks like qsort is
> being used incorrectly.

This seems to fix it for me, but I don't understand why. Apparently, a
pointer subtraction doesn't result in a signed value?? In any case,
the sort on those arrays wasn't correct. Comments?

Hello,

On 24.07.2012 18:56, Steven Bosscher wrote:
> On Tue, Jul 24, 2012 at 4:37 PM, Steven Bosscher<stevenb.gcc@gmail.com>  wrote:
>> On Tue, Jul 24, 2012 at 3:08 PM, Uros Bizjak<ubizjak@gmail.com>  wrote:
>>> This patch (r189803) regressed a bunch of tests on x86_64 [1], [2].
>>>
>>> [1] http://gcc.gnu.org/ml/gcc-testresults/2012-07/msg02066.html
>>> [2] http://gcc.gnu.org/ml/gcc-regression/2012-07/msg00177.html
>>
>> These are all selective-scheduler test cases. It looks like qsort is
>> being used incorrectly.
>
> This seems to fix it for me, but I don't understand why. Apparently, a
> pointer subtraction doesn't result in a signed value?? In any case,
> the sort on those arrays wasn't correct. Comments?
>
> Index: sel-sched-ir.c
> ===================================================================
> --- sel-sched-ir.c      (revision 189808)
> +++ sel-sched-ir.c      (working copy)
> @@ -954,7 +954,9 @@ return_regset_to_pool (regset rs)
>   static int
>   cmp_v_in_regset_pool (const void *x, const void *xx)
>   {
> -  return *((const regset *) x) - *((const regset *) xx);
> +  ptrdiff_t d = (ptrdiff_t) *((const regset *) x);
> +  ptrdiff_t dd = (ptrdiff_t) *((const regset *) xx);
> +  return d - dd;

AFAIR the qsort is just for getting a stable ordering over two pools to 
find the leaked regsets afterwards, the type of ordering doesn't matter. 
Anyways, how come this is related to your patch?  We don't use statistics 
in sel-sched...  Something got miscompiled?

Andrey


>   }
>   #endif

> AFAIR the qsort is just for getting a stable ordering over two pools to find
> the leaked regsets afterwards, the type of ordering doesn't matter.

What matters is that the compare function gives a reliable result. You
can't subtract pointers like that for qsort.

After consulting the experts on IRC, I'm going with a fix along the
lines of "return (x == y ? 0 : (x < y ? -1 : 1));".

> Anyways,
> how come this is related to your patch?  We don't use statistics in
> sel-sched...  Something got miscompiled?

No, just allocated slightly differently. A bitmap_head is one pointer
bigger than before. I'm unsure how that causes this problem, though. I
suspect you would have seen the same failure with GATHER_STATISTICS
enabled.

Ciao!
Steven

On 24.07.2012 21:13, Steven Bosscher wrote:
>> AFAIR the qsort is just for getting a stable ordering over two pools to find
>> the leaked regsets afterwards, the type of ordering doesn't matter.
>
> What matters is that the compare function gives a reliable result. You
> can't subtract pointers like that for qsort.
>
> After consulting the experts on IRC, I'm going with a fix along the
> lines of "return (x == y ? 0 : (x < y ? -1 : 1));".

Yeah, I agree the code was dubious, so thanks for uncovering this.  If you 
have already tested the patch, consider it preapproved, otherwise I can fix 
it on this week.

>
>> Anyways,
>> how come this is related to your patch?  We don't use statistics in
>> sel-sched...  Something got miscompiled?
>
> No, just allocated slightly differently. A bitmap_head is one pointer
> bigger than before. I'm unsure how that causes this problem, though. I
> suspect you would have seen the same failure with GATHER_STATISTICS
> enabled.

Still interesting to know why your first patch fixed the issue in the first 
place.  I will try taking a look at the generated code in my spare time.

Yours,
Andrey

>
> Ciao!
> Steven
>

On Tue, Jul 24, 2012 at 7:13 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>> AFAIR the qsort is just for getting a stable ordering over two pools to find
>> the leaked regsets afterwards, the type of ordering doesn't matter.
>
> What matters is that the compare function gives a reliable result. You
> can't subtract pointers like that for qsort.
>
> After consulting the experts on IRC, I'm going with a fix along the
> lines of "return (x == y ? 0 : (x < y ? -1 : 1));".
>
>> Anyways,
>> how come this is related to your patch?  We don't use statistics in
>> sel-sched...  Something got miscompiled?
>
> No, just allocated slightly differently. A bitmap_head is one pointer
> bigger than before. I'm unsure how that causes this problem, though. I
> suspect you would have seen the same failure with GATHER_STATISTICS
> enabled.

Oh, bigger bitmap_head?  That's bad ... :/  So much for '#ifdefs are bad' :/

Richard.

> Ciao!
> Steven

On Wed, Jul 25, 2012 at 1:49 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> Oh, bigger bitmap_head?  That's bad ... :/  So much for '#ifdefs are bad' :/

Bigger bitmap_head isn't a problem. A bigger bitmap_element would be bad.

For GGC allocated bitmaps, nothing changed (rounding, etc.).

If you think it's a problem anyway, I can eliminate desc from bitmap head...

Ciao!
Steven

On Wed, Jul 25, 2012 at 1:59 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Wed, Jul 25, 2012 at 1:49 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> Oh, bigger bitmap_head?  That's bad ... :/  So much for '#ifdefs are bad' :/
>
> Bigger bitmap_head isn't a problem. A bigger bitmap_element would be bad.
>
> For GGC allocated bitmaps, nothing changed (rounding, etc.).

I think we have extra buckets for 32bytes (before) and 40bytes (after).

> If you think it's a problem anyway, I can eliminate desc from bitmap head...

We embed bitmap_heads in core DF data structures for example, so eliminating
it would be nice.

Thanks,
Richard.

> Ciao!
> Steven