[Mesa-dev] [PATCH 03/18] ac: add load_tes_inputs() to the abi
Nicolai Hähnle
nhaehnle at gmail.com
Tue Dec 12 12:00:48 UTC 2017
On 12.12.2017 11:33, Timothy Arceri wrote:
> On 12/12/17 21:21, Timothy Arceri wrote:
>> On 12/12/17 20:42, Nicolai Hähnle wrote:
>>> On 11.12.2017 03:43, Timothy Arceri wrote:
>>>> ---
>>>> src/amd/common/ac_nir_to_llvm.c | 66
>>>> +++++++++++++++++++++-----------
>>>> src/amd/common/ac_shader_abi.h | 12 ++++++
>>>> src/gallium/drivers/radeonsi/si_shader.c | 1 +
>>>> 3 files changed, 57 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/src/amd/common/ac_nir_to_llvm.c
>>>> b/src/amd/common/ac_nir_to_llvm.c
>>>> index 663b27d265a..78ae25ee147 100644
>>>> --- a/src/amd/common/ac_nir_to_llvm.c
>>>> +++ b/src/amd/common/ac_nir_to_llvm.c
>>>> @@ -2840,53 +2840,51 @@ store_tcs_output(struct nir_to_llvm_context
>>>> *ctx,
>>>> }
>>>> if (writemask == 0xF) {
>>>> ac_build_buffer_store_dword(&ctx->ac,
>>>> ctx->hs_ring_tess_offchip, src, 4,
>>>> buf_addr, ctx->oc_lds,
>>>> (base * 4), 1, 0, true, false);
>>>> }
>>>> }
>>>> static LLVMValueRef
>>>> -load_tes_input(struct nir_to_llvm_context *ctx,
>>>> - const nir_intrinsic_instr *instr)
>>>> +load_tes_input(struct ac_shader_abi *abi,
>>>> + LLVMValueRef vertex_index,
>>>> + LLVMValueRef param_index,
>>>> + unsigned const_index,
>>>> + unsigned location,
>>>> + unsigned driver_location,
>>>> + LLVMTypeRef type,
>>>> + unsigned component,
>>>> + unsigned num_components,
>>>> + bool is_patch,
>>>> + bool is_compact)
>>>> {
>>>> + struct nir_to_llvm_context *ctx =
>>>> nir_to_llvm_context_from_abi(abi);
>>>> LLVMValueRef buf_addr;
>>>> LLVMValueRef result;
>>>> - LLVMValueRef vertex_index = NULL;
>>>> - LLVMValueRef indir_index = NULL;
>>>> - unsigned const_index = 0;
>>>> - unsigned param;
>>>> - const bool per_vertex =
>>>> nir_is_per_vertex_io(instr->variables[0]->var, ctx->stage);
>>>> - const bool is_compact = instr->variables[0]->var->data.compact;
>>>> + unsigned param = shader_io_get_unique_index(location);
>>>> - get_deref_offset(ctx->nir, instr->variables[0],
>>>> - false, NULL, per_vertex ? &vertex_index : NULL,
>>>> - &const_index, &indir_index);
>>>> - param =
>>>> shader_io_get_unique_index(instr->variables[0]->var->data.location);
>>>> - if (instr->variables[0]->var->data.location ==
>>>> VARYING_SLOT_CLIP_DIST0 &&
>>>> - is_compact && const_index > 3) {
>>>> + if (location == VARYING_SLOT_CLIP_DIST0 && is_compact &&
>>>> const_index > 3) {
>>>> const_index -= 3;
>>>
>>> So... this is in the original code as well, but why is that -= 3
>>> instead of 4?
>>
>> Honestly since I didn't have to touch this code I haven't really tried
>> to understand it. Seems like a classic example of why you shouldn't
>> use magic numbers without a comment.
Absolutely, and I'd not recommend changing it in this patch, but it's
something to keep in mind. It's a radv-code path anyway, so officially I
don't care ;)
>>>> param++;
>>>> }
>>>> - unsigned comp = instr->variables[0]->var->data.location_frac;
>>>> buf_addr = get_tcs_tes_buffer_address_params(ctx, param,
>>>> const_index,
>>>> - is_compact, vertex_index, indir_index);
>>>> + is_compact, vertex_index, param_index);
>>>> - LLVMValueRef comp_offset = LLVMConstInt(ctx->ac.i32, comp * 4,
>>>> false);
>>>> + LLVMValueRef comp_offset = LLVMConstInt(ctx->ac.i32, component
>>>> * 4, false);
>>>> buf_addr = LLVMBuildAdd(ctx->builder, buf_addr, comp_offset, "");
>>>> - result = ac_build_buffer_load(&ctx->ac,
>>>> ctx->hs_ring_tess_offchip, instr->num_components, NULL,
>>>> + result = ac_build_buffer_load(&ctx->ac,
>>>> ctx->hs_ring_tess_offchip, num_components, NULL,
>>>> buf_addr, ctx->oc_lds, is_compact ? (4 *
>>>> const_index) : 0, 1, 0, true, false);
>>>> - result = trim_vector(&ctx->ac, result, instr->num_components);
>>>> - result = LLVMBuildBitCast(ctx->builder, result,
>>>> get_def_type(ctx->nir, &instr->dest.ssa), "");
>>>> + result = trim_vector(&ctx->ac, result, num_components);
>>>> return result;
>>>> }
>>>> static LLVMValueRef
>>>> load_gs_input(struct ac_shader_abi *abi,
>>>> unsigned location,
>>>> unsigned driver_location,
>>>> unsigned component,
>>>> unsigned num_components,
>>>> unsigned vertex_index,
>>>> @@ -2988,22 +2986,45 @@ static LLVMValueRef visit_load_var(struct
>>>> ac_nir_context *ctx,
>>>> get_deref_offset(ctx, instr->variables[0], vs_in, NULL, NULL,
>>>> &const_index, &indir_index);
>>>> if (instr->dest.ssa.bit_size == 64)
>>>> ve *= 2;
>>>> switch (instr->variables[0]->var->data.mode) {
>>>> case nir_var_shader_in:
>>>> if (ctx->stage == MESA_SHADER_TESS_CTRL)
>>>> return load_tcs_input(ctx->nctx, instr);
>>>> - if (ctx->stage == MESA_SHADER_TESS_EVAL)
>>>> - return load_tes_input(ctx->nctx, instr);
>>>> + if (ctx->stage == MESA_SHADER_TESS_EVAL) {
>>>> + LLVMValueRef result;
>>>> + LLVMValueRef vertex_index = NULL;
>>>> + LLVMValueRef indir_index = NULL;
>>>> + unsigned const_index = 0;
>>>> + unsigned location =
>>>> instr->variables[0]->var->data.location;
>>>> + unsigned driver_location =
>>>> instr->variables[0]->var->data.driver_location;
>>>> + const bool is_patch =
>>>> instr->variables[0]->var->data.patch;
>>>> + const bool is_compact =
>>>> instr->variables[0]->var->data.compact;
>>>> +
>>>> + get_deref_offset(ctx, instr->variables[0],
>>>> + false, NULL, is_patch ? NULL : &vertex_index,
>>>> + &const_index, &indir_index);
>>>> +
>>>> + result = ctx->abi->load_tess_inputs(ctx->abi,
>>>> vertex_index, indir_index,
>>>> + const_index, location,
>>>> driver_location,
>>>> + nir2llvmtype(ctx,
>>>> instr->variables[0]->var->type),
>>>> + instr->variables[0]->var->data.location_frac,
>>>> + instr->num_components,
>>>> + is_patch, is_compact);
>>>> + return ctx->nctx ?
>>>> + LLVMBuildBitCast(ctx->nctx->builder, result,
>>>> get_def_type(ctx, &instr->dest.ssa), "") :
>>>> + result;
>>>
>>> Why is this conditional on ctx->nctx? Could and should probably just
>>> use ctx->ac.builder...
>>
>>
>> I'm fairly certain the bitcast caused problems for the radeonsi path.
>> Maybe this is not the best solution but this is one of a couple of
>> places where the backends need to be massaged in slightly different
>> ways, checking for ctx->nctx is a convenient way to switch paths.
>
> Actually this might have been the same issue I described in my reply to
> the cover i.e. using get_def_type(ctx, &instr->dest.ssa) causing strange
> behavior, I guess I could try again to debug the issue.
Yeah, that would be good to understand better.
Thanks,
Nicolai
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
More information about the mesa-dev
mailing list