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.cindex 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. _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
