[Mesa-dev] [PATCH 3/4] [rfc] gallivm/llvmpipe dynamic samplers support.

[Dave Airlie]

On 19 April 2016 at 03:40, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 18.04.2016 um 04:49 schrieb Dave Airlie:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> This is a first attempt at adding support for dynamic indexing
>> of samplers to llvmpipe. This is needed for ARB_gpu_shader5 support.
>>
>> This uses the sampler function generator to generate functions
>> for all samplers, then uses if statements to pick which one to call.
>>
>> This passes all the tests in piglit except a couple of non-uniform
>> ones which g
> I can't quite parse this fully ;-).
>
Doh I wrote half a commit msg, got distracted by query size and forgot
to go back.

>> +
>> +         for (i = 0; i < sampler->dynamic_state.num_samplers; i++) {
> Indentation.
> And I think this should really make an effort to only do this for
> samplers which are actually dyanmically indexed, for the possible
> range(s) (as they need to be declared as an array if I'm not mistaken).
> I'm not entirely sure, but I believe this could even crash otherwise
> potentially, due to trying to construct lookup functions with
> incompatible combinations of parameters.

No unfortunately we don't declare samplers as an array, I think I should
fix that at some point as I've come to this point a few times and would have
really liked to have that information, at least for bounds checking.

>
>> +         struct lp_sampler_params unit_params = *params;
>> +
>> +         unit_params.texture_index = i;
>> +         unit_params.sampler_index = i;
> Of course, the need to actually override texture and sampler index is
> why I never even tried to implement it - for d3d10 sample opcodes, it
> looked simply unfeasible as you'd have to generate the product of
> num_samplers/num_textures. So I figured this needed way more work...
> Though actually thinking about this, only need to be able to index into
> resources, not samplers, there, so it should work as well...
> I can't really make sense out of the msdn docs though.
> But with d3d12 you actually could have legitimate per-pixel resource
> indices (yuck!!!) - with the usual caveat about calculated lods.
> But I can't actually find anything that says dynamic indexing into
> resources even works at all for d3d11 now.

Yup I pretty much ignored SAMPLE* and D3D due to lack of any clue.

Though really generating all the functions in the world probably isn't
going to be that brutal. but it would be good to know if you can indirect
both.

>
>> +         lp_build_sample_soa(&sampler->dynamic_state.static_state[i].texture_state,
>> +                             &sampler->dynamic_state.static_state[i].sampler_state,
>> +                             &sampler->dynamic_state.base,
>> +                             gallivm, &unit_params);
> And I think really all of this should move to the actual sample code.

It's kinda messier to do that, since we passing
dynamic_state.static_state, and I don't think
we can access it anywhere else except here.

>> +      indirect_matches = LLVMBuildBitCast(builder, indirect_matches, LLVMIntTypeInContext(gallivm->context, params->type.length), "");
>> +      indirect_matches = LLVMBuildICmp(builder, LLVMIntNE,
>> +                                       indirect_matches, LLVMConstNull(LLVMIntTypeInContext(gallivm->context, params->type.length)), "");
>>
> This isn't quite right (I suppose that's what the commit message wanted
> to say but was cut short...).
> From what I understand, you really need to examine the exec mask (not
> sure if the alive pixel mask is also needed) and pick your (scalar)
> index from a element which is in the current exec mask. I guess that's
> another parameter for the sampler params...
> And fwiw I think doing that with a switch statement in the end instead
> of if ladders would make far more sense.

Ah yes I expect you might be right in that and that is why the two
tests are failing, I'll see if I can work it out.

A switch statement in the end means having to keep track of all the
function pointers, whereas doing it with
separate if statements makes it a lot easier to generate the code
without having to keep copies of all the individual
samplers to build the switch at the end. Though I suppose it might be
possible to wrap it at a higher level.

>> +   if (params.sampler_is_indirect)
>> +       params.indirect_index = get_indirect_index(bld, inst->Src[sampler_reg].Register.File, inst->Src[sampler_reg].Register.Index, &inst->Src[sampler_reg].Indirect);
> Does this ensure that the index doesn't exceed max sampler index?
> Hopefully yes, but if not you need to add some MIN here.

Ah I'll check that.

Thanks,
Dave.