<div dir="ltr">On 2 December 2013 11:39, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">--- src/mesa/drivers/dri/i965/brw_fs.h | 9 ------ src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 38 ++++------------------ src/mesa/drivers/dri/i965/brw_vec4.h | 9 ------ src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 40 ++++-------------------- 4 files changed, 12 insertions(+), 84 deletions(-) </blockquote><div> </div><div>The commit message should note that you've also loosened the restrictions on atomic operations--the surf_index used with an atomic operatoin no longer needs to be an immediate constant. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h index 4ada075..7bfa9fd 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.h +++ b/src/mesa/drivers/dri/i965/brw_fs.h @@ -601,15 +601,6 @@ private: struct brw_reg offset, struct brw_reg value); - void generate_untyped_atomic(fs_inst *inst, - struct brw_reg dst, - struct brw_reg atomic_op, - struct brw_reg surf_index); - - void generate_untyped_surface_read(fs_inst *inst, - struct brw_reg dst, - struct brw_reg surf_index); - void patch_discard_jumps_to_fb_writes(); struct brw_context *brw; diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp index 4eb651f..0d50051 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp @@ -1255,36 +1255,6 @@ fs_generator::generate_shader_time_add(fs_inst *inst, } void -fs_generator::generate_untyped_atomic(fs_inst *inst, struct brw_reg dst, - struct brw_reg atomic_op, - struct brw_reg surf_index) -{ - assert(atomic_op.file == BRW_IMMEDIATE_VALUE && - atomic_op.type == BRW_REGISTER_TYPE_UD && - surf_index.file == BRW_IMMEDIATE_VALUE && - surf_index.type == BRW_REGISTER_TYPE_UD); - - brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf), - surf_index, atomic_op.dw1.ud, - inst->mlen, true); - - brw_mark_surface_used(&c->prog_data.base, surf_index.dw1.ud); -} - -void -fs_generator::generate_untyped_surface_read(fs_inst *inst, struct brw_reg dst, - struct brw_reg surf_index) -{ - assert(surf_index.file == BRW_IMMEDIATE_VALUE && - surf_index.type == BRW_REGISTER_TYPE_UD); - - brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf), - surf_index, inst->mlen, 1); - - brw_mark_surface_used(&c->prog_data.base, surf_index.dw1.ud); -} - -void fs_generator::generate_code(exec_list *instructions) { int last_native_insn_offset = p->next_insn_offset; @@ -1709,11 +1679,15 @@ fs_generator::generate_code(exec_list *instructions) break; case SHADER_OPCODE_UNTYPED_ATOMIC: - generate_untyped_atomic(inst, dst, src[0], src[1]); + assert(src[1].file == BRW_IMMEDIATE_VALUE); + brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf), + src[0], src[1].dw1.ud, inst->mlen, true); </blockquote><div> </div><div>I've seen the pattern crop up several times in this series now of: 1. assert that a brw_reg is an immediate value, 2. extract reg.dw1.ud. How about if we make a function to do this (maybe brw_get_imm_ud())? I think that will be a lot more readable since it won't force people to remember that the immediate value is stored in brw_reg::dw1. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> break; case SHADER_OPCODE_UNTYPED_SURFACE_READ: - generate_untyped_surface_read(inst, dst, src[0]); + assert(src[1].file == BRW_IMMEDIATE_VALUE); + brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf), + src[0], inst->mlen, src[1].dw1.ud); break; </blockquote><div> </div><div>In both of the above two cases, generate_untyped_{atomic,surface_read} used to call brw_mark_surface_used(). That's not being called anymore. </div><div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> case FS_OPCODE_SET_SIMD4X2_OFFSET: diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h index 355d497..7e07929 100644 --- a/src/mesa/drivers/dri/i965/brw_vec4.h +++ b/src/mesa/drivers/dri/i965/brw_vec4.h @@ -666,15 +666,6 @@ private: void generate_unpack_flags(vec4_instruction *inst, struct brw_reg dst); - void generate_untyped_atomic(vec4_instruction *inst, - struct brw_reg dst, - struct brw_reg atomic_op, - struct brw_reg surf_index); - - void generate_untyped_surface_read(vec4_instruction *inst, - struct brw_reg dst, - struct brw_reg surf_index); - struct brw_context *brw; struct brw_compile *p; diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp index 3ac45a9..d29c3dd 100644 --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp @@ -847,38 +847,6 @@ vec4_generator::generate_pull_constant_load_gen7(vec4_instruction *inst, brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud); } -void -vec4_generator::generate_untyped_atomic(vec4_instruction *inst, - struct brw_reg dst, - struct brw_reg atomic_op, - struct brw_reg surf_index) -{ - assert(atomic_op.file == BRW_IMMEDIATE_VALUE && - atomic_op.type == BRW_REGISTER_TYPE_UD && - surf_index.file == BRW_IMMEDIATE_VALUE && - surf_index.type == BRW_REGISTER_TYPE_UD); - - brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf), - surf_index, atomic_op.dw1.ud, - inst->mlen, true); - - brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud); -} - -void -vec4_generator::generate_untyped_surface_read(vec4_instruction *inst, - struct brw_reg dst, - struct brw_reg surf_index) -{ - assert(surf_index.file == BRW_IMMEDIATE_VALUE && - surf_index.type == BRW_REGISTER_TYPE_UD); - - brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf), - surf_index, inst->mlen, 1); - - brw_mark_surface_used(&prog_data->base, surf_index.dw1.ud); -} - /** * Generate assembly for a Vec4 IR instruction. * @@ -1193,11 +1161,15 @@ vec4_generator::generate_vec4_instruction(vec4_instruction *instruction, break; case SHADER_OPCODE_UNTYPED_ATOMIC: - generate_untyped_atomic(inst, dst, src[0], src[1]); + assert(src[1].file == BRW_IMMEDIATE_VALUE); + brw_untyped_atomic(p, dst, brw_message_reg(inst->base_mrf), + src[0], src[1].dw1.ud, inst->mlen, true); break; case SHADER_OPCODE_UNTYPED_SURFACE_READ: - generate_untyped_surface_read(inst, dst, src[0]); + assert(src[1].file == BRW_IMMEDIATE_VALUE); + brw_untyped_surface_read(p, dst, brw_message_reg(inst->base_mrf), + src[0], inst->mlen, src[1].dw1.ud); break; </blockquote><div> </div><div>Same problem about brw_mark_surface_used() here. </div><div>With those issues addressed, this patch is: Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div></div></div></div>