[Mesa-dev] [PATCH 4/4] radeonsi: add instance divisor support

[Christian König]

Am 26.03.2013 18:03, schrieb Michel Dänzer:
> On Die, 2013-03-26 at 17:37 +0100, Christian König wrote:
>> Am 26.03.2013 15:56, schrieb Michel Dänzer:
>>> On Die, 2013-03-26 at 14:51 +0100, Christian König wrote:
>>>> From: Christian König <christian.koenig at amd.com>
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> [...]
>>>> diff --git a/src/gallium/drivers/radeonsi/radeonsi_shader.h b/src/gallium/drivers/radeonsi/radeonsi_shader.h
>>>> index 9dae742..e09f297 100644
>>>> --- a/src/gallium/drivers/radeonsi/radeonsi_shader.h
>>>> +++ b/src/gallium/drivers/radeonsi/radeonsi_shader.h
>>>> @@ -111,13 +111,18 @@ struct si_shader {
>>>>    	unsigned		nr_cbufs;
>>>>    };
>>>>    
>>>> -struct si_shader_key {
>>>> -	unsigned		export_16bpc:8;
>>>> -	unsigned		nr_cbufs:4;
>>>> -	unsigned		color_two_side:1;
>>>> -	unsigned		alpha_func:3;
>>>> -	unsigned		flatshade:1;
>>>> -	float			alpha_ref;
>>>> +union si_shader_key {
>>>> +	struct {
>>>> +		unsigned	export_16bpc:8;
>>>> +		unsigned	nr_cbufs:4;
>>>> +		unsigned	color_two_side:1;
>>>> +		unsigned	alpha_func:3;
>>>> +		unsigned	flatshade:1;
>>>> +		float		alpha_ref;
>>>> +	} ps;
>>>> +	struct {
>>>> +		unsigned	instance_divisors[PIPE_MAX_ATTRIBS];
>>>> +	} vs;
>>>>    };
>>> This grows the shader key from 8 to 128 bytes. I don't suppose the
>>> instance divisors could be encoded in a more compact way? E.g. loading
>>> the divisor values from constants and only tracking which elements use a
>>> divisor in a bitmask in the key.
>> Considered that also, and I have two problems with that approach:
>> 1. While immediates are converted to shifts & muls, dividing even by a
>> constant in the shader isn't cheap.
> Is that really significant? How much work would it be to come up with a
> worst case test and measure the difference?

Well no idea how to measure that on SI, but when I implemented the same 
feature on R600 the difference between using reciprocal and mul compared 
to mulhi where quite significant.

>
>> How about storing only a byte for the instance_divisor? That limit's the
>> divisor to a modulo of 256, but I don't think that would be so extremly bad.
> I have no idea what the impact of that would be. What happens if an app
> tries to use a divisor >= 256?

It probably would select the wrong shader :(

>> That would reduce the key to 32 bytes instead.
> Still seems kind of big.

Ok how about the following compromise: First we use a short for the 
instance divisor, that makes the key 32 bytes in size and should leave 
enough room for larger instance divisors, and second we don't copy the 
key around so much any more.

Regards,
Christian.