Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-08 Thread Marek Olšák 
On Thu, Sep 8, 2016 at 3:56 AM, Michel Dänzer  wrote:
> On 07/09/16 11:01 PM, Nicolai Hähnle wrote:
>> On 07.09.2016 12:23, Marek Olšák wrote:
>>> On Wed, Sep 7, 2016 at 11:47 AM, Michel Dänzer 
>>> wrote:
 On 07/09/16 06:02 PM, Marek Olšák wrote:
>
> Based on the comments so far, it looks like all annotations in the
> patch are very well placed, so I don't know what the fuss is about.

 As I said, there's no question about the first three annotations, but
 for the last two we can't know whether they help or actually hurt
 without measuring it. That's the whole point.
>>>
>>> OK. The last two are so rare that their likeliness is below 1%. The
>>> conditions are true when discarding CMASK or DCC, changing the micro
>>> tile mode of a texture, reallocating texture storage, and degrading
>>> the tile mode to linear. Some of those are caused by SDMA doing a
>>> whole-layer overwrite, a texture transfer, or a buffer handle export.
>>> Some of them can occur only once during the lifetime of a texture.
>>> They are very rare during normal gaming, which just consists of state
>>> changes and draw calls and nothing in between. I don't know about
>>> microbenchmarks.
>>
>> FWIW, though I haven't said so explicitly (other than sending my R-b
>> while this discussion was already going on), I also think those last two
>> cases are fine. We probably shouldn't go overboard with the (un)likely,
>> but really, the patch is fine as is.
>
> Okay, thanks.
>
> FWIW, my concern wasn't only about this patch itself but also about not
> wasting time on similar changes in the future.

30 seconds to write and commit it, and 10 seconds to review it isn't
gonna make a difference on anybody. Also, it should be kinda obvious
that I work more than 40 hours a week. I can do whatever I want in my
non-job time dedicated to drivers.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-07 Thread Michel Dänzer 
On 07/09/16 11:01 PM, Nicolai Hähnle wrote:
> On 07.09.2016 12:23, Marek Olšák wrote:
>> On Wed, Sep 7, 2016 at 11:47 AM, Michel Dänzer 
>> wrote:
>>> On 07/09/16 06:02 PM, Marek Olšák wrote:

 Based on the comments so far, it looks like all annotations in the
 patch are very well placed, so I don't know what the fuss is about.
>>>
>>> As I said, there's no question about the first three annotations, but
>>> for the last two we can't know whether they help or actually hurt
>>> without measuring it. That's the whole point.
>>
>> OK. The last two are so rare that their likeliness is below 1%. The
>> conditions are true when discarding CMASK or DCC, changing the micro
>> tile mode of a texture, reallocating texture storage, and degrading
>> the tile mode to linear. Some of those are caused by SDMA doing a
>> whole-layer overwrite, a texture transfer, or a buffer handle export.
>> Some of them can occur only once during the lifetime of a texture.
>> They are very rare during normal gaming, which just consists of state
>> changes and draw calls and nothing in between. I don't know about
>> microbenchmarks.
> 
> FWIW, though I haven't said so explicitly (other than sending my R-b
> while this discussion was already going on), I also think those last two
> cases are fine. We probably shouldn't go overboard with the (un)likely,
> but really, the patch is fine as is.

Okay, thanks.

FWIW, my concern wasn't only about this patch itself but also about not
wasting time on similar changes in the future.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-07 Thread Nicolai Hähnle 

On 07.09.2016 12:23, Marek Olšák wrote:

On Wed, Sep 7, 2016 at 11:47 AM, Michel Dänzer  wrote:

On 07/09/16 06:02 PM, Marek Olšák wrote:


Based on the comments so far, it looks like all annotations in the
patch are very well placed, so I don't know what the fuss is about.


As I said, there's no question about the first three annotations, but
for the last two we can't know whether they help or actually hurt
without measuring it. That's the whole point.


OK. The last two are so rare that their likeliness is below 1%. The
conditions are true when discarding CMASK or DCC, changing the micro
tile mode of a texture, reallocating texture storage, and degrading
the tile mode to linear. Some of those are caused by SDMA doing a
whole-layer overwrite, a texture transfer, or a buffer handle export.
Some of them can occur only once during the lifetime of a texture.
They are very rare during normal gaming, which just consists of state
changes and draw calls and nothing in between. I don't know about
microbenchmarks.


FWIW, though I haven't said so explicitly (other than sending my R-b 
while this discussion was already going on), I also think those last two 
cases are fine. We probably shouldn't go overboard with the (un)likely, 
but really, the patch is fine as is.


Nicolai



Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-07 Thread Marek Olšák 
On Wed, Sep 7, 2016 at 11:47 AM, Michel Dänzer  wrote:
> On 07/09/16 06:02 PM, Marek Olšák wrote:
>>
>> Based on the comments so far, it looks like all annotations in the
>> patch are very well placed, so I don't know what the fuss is about.
>
> As I said, there's no question about the first three annotations, but
> for the last two we can't know whether they help or actually hurt
> without measuring it. That's the whole point.

OK. The last two are so rare that their likeliness is below 1%. The
conditions are true when discarding CMASK or DCC, changing the micro
tile mode of a texture, reallocating texture storage, and degrading
the tile mode to linear. Some of those are caused by SDMA doing a
whole-layer overwrite, a texture transfer, or a buffer handle export.
Some of them can occur only once during the lifetime of a texture.
They are very rare during normal gaming, which just consists of state
changes and draw calls and nothing in between. I don't know about
microbenchmarks.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-07 Thread Michel Dänzer 
On 07/09/16 06:02 PM, Marek Olšák wrote:
> 
> Based on the comments so far, it looks like all annotations in the
> patch are very well placed, so I don't know what the fuss is about.

As I said, there's no question about the first three annotations, but
for the last two we can't know whether they help or actually hurt
without measuring it. That's the whole point.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-07 Thread Marek Olšák 
On Wed, Sep 7, 2016 at 2:23 AM, Michel Dänzer  wrote:
> On 06/09/16 08:33 PM, Marek Olšák wrote:
>> On Sep 6, 2016 12:03 PM, "Michel Dänzer" > > wrote:
>>> On 06/09/16 06:04 PM, Marek Olšák wrote:
>>> > On Tue, Sep 6, 2016 at 3:54 AM, Michel Dänzer > > wrote:
>>> >> On 06/09/16 07:46 AM, Marek Olšák wrote:
>>> >>> From: Marek Olšák >
>>> >>
>>> >> Did you measure any significant performance boost with this change?
>>> >
>>> > I didn't measure anything.
>>> >
>>> >> Otherwise, using (un)likely can be bad because it can defeat the CPU's
>>> >> branch prediction, which tends to be pretty good these days.
>>> >
>>> > I'm not an expert on that, but it doesn't seem to be the case
>>> > according to other people's comments here.
>>>
>>> My main point (which Gustaw seems to agree with) is that (un)likely
>>> should only be used when measurements show that they have a positive
>> effect.
>>
>> I agree with you, but do you always measure the effect of unlikely? I
>> almost never do and I just use it instinctively like most people do. Due
>> to our manpower constraints, we can't even afford to measure performance
>> for much bigger changes than this.
>
> So let's spend our manpower on more important things than (un)likely
> annotations. :)

Based on the comments so far, it looks like all annotations in the
patch are very well placed, so I don't know what the fuss is about.

Let's spend our manpower on more important things than bikeshedding
unlikely annotations that has already consumed much more time than me
writing this trivial patch.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-06 Thread Rob Clark 
On Tue, Sep 6, 2016 at 8:23 PM, Michel Dänzer  wrote:
> On 06/09/16 08:33 PM, Marek Olšák wrote:
>> On Sep 6, 2016 12:03 PM, "Michel Dänzer" > > wrote:
>>> On 06/09/16 06:04 PM, Marek Olšák wrote:
>>> > On Tue, Sep 6, 2016 at 3:54 AM, Michel Dänzer > > wrote:
>>> >> On 06/09/16 07:46 AM, Marek Olšák wrote:
>>> >>> From: Marek Olšák >
>>> >>
>>> >> Did you measure any significant performance boost with this change?
>>> >
>>> > I didn't measure anything.
>>> >
>>> >> Otherwise, using (un)likely can be bad because it can defeat the CPU's
>>> >> branch prediction, which tends to be pretty good these days.
>>> >
>>> > I'm not an expert on that, but it doesn't seem to be the case
>>> > according to other people's comments here.
>>>
>>> My main point (which Gustaw seems to agree with) is that (un)likely
>>> should only be used when measurements show that they have a positive
>> effect.
>>
>> I agree with you, but do you always measure the effect of unlikely? I
>> almost never do and I just use it instinctively like most people do. Due
>> to our manpower constraints, we can't even afford to measure performance
>> for much bigger changes than this.
>
> So let's spend our manpower on more important things than (un)likely
> annotations. :)
>

I guess it is still a reasonable thing to use annotations in places
that should really be gl edge cases where we don't want to penalize
if(false) case by jumping over multiple cache lines of instructions
(ie. I still plan to use it for the if(unlikely(samplerExternalOES))
stuff to deal with YUV external images).. but in cases where there is
more doubt probably not worth using them.

BR,
-R
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-06 Thread Michel Dänzer 
On 06/09/16 08:33 PM, Marek Olšák wrote:
> On Sep 6, 2016 12:03 PM, "Michel Dänzer"  > wrote:
>> On 06/09/16 06:04 PM, Marek Olšák wrote:
>> > On Tue, Sep 6, 2016 at 3:54 AM, Michel Dänzer  > wrote:
>> >> On 06/09/16 07:46 AM, Marek Olšák wrote:
>> >>> From: Marek Olšák >
>> >>
>> >> Did you measure any significant performance boost with this change?
>> >
>> > I didn't measure anything.
>> >
>> >> Otherwise, using (un)likely can be bad because it can defeat the CPU's
>> >> branch prediction, which tends to be pretty good these days.
>> >
>> > I'm not an expert on that, but it doesn't seem to be the case
>> > according to other people's comments here.
>>
>> My main point (which Gustaw seems to agree with) is that (un)likely
>> should only be used when measurements show that they have a positive
> effect.
> 
> I agree with you, but do you always measure the effect of unlikely? I
> almost never do and I just use it instinctively like most people do. Due
> to our manpower constraints, we can't even afford to measure performance
> for much bigger changes than this.

So let's spend our manpower on more important things than (un)likely
annotations. :)


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-06 Thread Marek Olšák 
On Sep 6, 2016 12:03 PM, "Michel Dänzer"  wrote:
>
> On 06/09/16 06:04 PM, Marek Olšák wrote:
> > On Tue, Sep 6, 2016 at 3:54 AM, Michel Dänzer 
wrote:
> >> On 06/09/16 07:46 AM, Marek Olšák wrote:
> >>> From: Marek Olšák 
> >>
> >> Did you measure any significant performance boost with this change?
> >
> > I didn't measure anything.
> >
> >> Otherwise, using (un)likely can be bad because it can defeat the CPU's
> >> branch prediction, which tends to be pretty good these days.
> >
> > I'm not an expert on that, but it doesn't seem to be the case
> > according to other people's comments here.
>
> My main point (which Gustaw seems to agree with) is that (un)likely
> should only be used when measurements show that they have a positive
effect.

I agree with you, but do you always measure the effect of unlikely? I
almost never do and I just use it instinctively like most people do. Due to
our manpower constraints, we can't even afford to measure performance for
much bigger changes than this.

Marek

Marek

>
>
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-06 Thread Michel Dänzer 
On 06/09/16 06:04 PM, Marek Olšák wrote:
> On Tue, Sep 6, 2016 at 3:54 AM, Michel Dänzer  wrote:
>> On 06/09/16 07:46 AM, Marek Olšák wrote:
>>> From: Marek Olšák 
>>
>> Did you measure any significant performance boost with this change?
> 
> I didn't measure anything.
> 
>> Otherwise, using (un)likely can be bad because it can defeat the CPU's
>> branch prediction, which tends to be pretty good these days.
> 
> I'm not an expert on that, but it doesn't seem to be the case
> according to other people's comments here.

My main point (which Gustaw seems to agree with) is that (un)likely
should only be used when measurements show that they have a positive effect.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-06 Thread Marek Olšák 
On Tue, Sep 6, 2016 at 3:54 AM, Michel Dänzer  wrote:
> On 06/09/16 07:46 AM, Marek Olšák wrote:
>> From: Marek Olšák 
>
> Did you measure any significant performance boost with this change?

I didn't measure anything.

> Otherwise, using (un)likely can be bad because it can defeat the CPU's
> branch prediction, which tends to be pretty good these days.

I'm not an expert on that, but it doesn't seem to be the case
according to other people's comments here.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-06 Thread Gustaw Smolarczyk 
2016-09-06 3:56 GMT+02:00 Ilia Mirkin :
> On Mon, Sep 5, 2016 at 9:54 PM, Michel Dänzer  wrote:
>> On 06/09/16 07:46 AM, Marek Olšák wrote:
>>> From: Marek Olšák 
>>
>> Did you measure any significant performance boost with this change?
>> Otherwise, using (un)likely can be bad because it can defeat the CPU's
>> branch prediction, which tends to be pretty good these days.
>
> Is there a way to affect the branch predictor on x86 with instruction
> encodings? I didn't think so. I was under the impression that all
> likely/unlikely did was to affect placement of the code, i.e. where
> the "if" code was placed.

If I may add to the discussion: there was a way to add branch
prediction hints to the instruction encoding (using x86 prefixes that
were to be ignored according to ISA), which was used by NetBurst
architecture (Pentium 4). It is no longer recognized by any modern
architecture and AFAIK compilers will not generate code that uses
them.

The compiler should be able to do two things using the (un)likely
hints (there might be more tricks I am not aware of):
1. Make the likely branch not jump. When the CPU executes a jump
without any branch prediction data cached for it, it assumes that it
doesn't jump.
2. Move the unlikely parts of code outside of a function or to the end
of a function. That increases instruction cache and fetch usage for
likely code.

In the end, it would be best to measure the performance of the (un)likely hints.

Regards,
Gustaw Smolarczyk

>
>   -ilia
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-05 Thread Ilia Mirkin 
On Mon, Sep 5, 2016 at 9:54 PM, Michel Dänzer  wrote:
> On 06/09/16 07:46 AM, Marek Olšák wrote:
>> From: Marek Olšák 
>
> Did you measure any significant performance boost with this change?
> Otherwise, using (un)likely can be bad because it can defeat the CPU's
> branch prediction, which tends to be pretty good these days.

Is there a way to affect the branch predictor on x86 with instruction
encodings? I didn't think so. I was under the impression that all
likely/unlikely did was to affect placement of the code, i.e. where
the "if" code was placed.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add more unlikely() uses into si_draw_vbo

2016-09-05 Thread Michel Dänzer 
On 06/09/16 07:46 AM, Marek Olšák wrote:
> From: Marek Olšák 

Did you measure any significant performance boost with this change?
Otherwise, using (un)likely can be bad because it can defeat the CPU's
branch prediction, which tends to be pretty good these days.


> - if (!sctx->vs_shader.cso) {
> + if (unlikely(!sctx->vs_shader.cso)) {
>   assert(0);
>   return;
>   }
> - if (!sctx->ps_shader.cso && (!rs || !rs->rasterizer_discard)) {
> + if (unlikely(!sctx->ps_shader.cso && (!rs || !rs->rasterizer_discard))) 
> {
>   assert(0);
>   return;
>   }
> - if (!!sctx->tes_shader.cso != (info->mode == PIPE_PRIM_PATCHES)) {
> + if (unlikely(!!sctx->tes_shader.cso != (info->mode == 
> PIPE_PRIM_PATCHES))) {
>   assert(0);
>   return;
>   }

These three are no-brainers though, as these conditions are normally
never true.


>   /* Re-emit the framebuffer state if needed. */
>   dirty_fb_counter = p_atomic_read(>b.screen->dirty_fb_counter);
> - if (dirty_fb_counter != sctx->b.last_dirty_fb_counter) {
> + if (unlikely(dirty_fb_counter != sctx->b.last_dirty_fb_counter)) {
>   sctx->b.last_dirty_fb_counter = dirty_fb_counter;
>   sctx->framebuffer.dirty_cbufs |=
>   ((1 << sctx->framebuffer.state.nr_cbufs) - 1);
>   sctx->framebuffer.dirty_zsbuf = true;
>   si_mark_atom_dirty(sctx, >framebuffer.atom);
>   }
>  
>   /* Invalidate & recompute texture descriptors if needed. */
>   dirty_tex_counter = 
> p_atomic_read(>b.screen->dirty_tex_descriptor_counter);
> - if (dirty_tex_counter != sctx->b.last_dirty_tex_descriptor_counter) {
> + if (unlikely(dirty_tex_counter != 
> sctx->b.last_dirty_tex_descriptor_counter)) {
>   sctx->b.last_dirty_tex_descriptor_counter = dirty_tex_counter;
>   si_update_all_texture_descriptors(sctx);
>   }

But these two might be better left to the CPU's branch prediction.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev