<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Aug 29, 2018 at 1:36 AM Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Friday, August 17, 2018 1:06:24 PM PDT Jason Ekstrand wrote:<br>
> ---<br>
> src/intel/compiler/brw_eu_defines.h | 3 +++<br>
> src/intel/compiler/brw_fs.cpp | 8 ++++++<br>
> src/intel/compiler/brw_fs_generator.cpp | 26 ++++++++++++++++---<br>
> src/intel/compiler/brw_fs_nir.cpp | 26 +++++++++++++++++++<br>
> .../compiler/brw_nir_lower_image_load_store.c | 15 +++++++++++<br>
> src/intel/compiler/brw_shader.cpp | 5 ++++<br>
> 6 files changed, 79 insertions(+), 4 deletions(-)<br>
> <br>
> diff --git a/src/intel/compiler/brw_eu_defines.h b/src/intel/compiler/brw_eu_defines.h<br>
> index 2289fc9be70..c36699d7676 100644<br>
> --- a/src/intel/compiler/brw_eu_defines.h<br>
> +++ b/src/intel/compiler/brw_eu_defines.h<br>
> @@ -354,6 +354,9 @@ enum opcode {<br>
> SHADER_OPCODE_SAMPLEINFO,<br>
> SHADER_OPCODE_SAMPLEINFO_LOGICAL,<br>
> <br>
> + SHADER_OPCODE_IMAGE_SIZE,<br>
> + SHADER_OPCODE_IMAGE_SIZE_LOGICAL,<br>
> +<br>
> /**<br>
> * Combines multiple sources of size 1 into a larger virtual GRF.<br>
> * For example, parameters for a send-from-GRF message. Or, updating<br>
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp<br>
> index 5b87991652d..1e1954270e8 100644<br>
> --- a/src/intel/compiler/brw_fs.cpp<br>
> +++ b/src/intel/compiler/brw_fs.cpp<br>
> @@ -4946,6 +4946,14 @@ fs_visitor::lower_logical_sends()<br>
> lower_sampler_logical_send(ibld, inst, SHADER_OPCODE_SAMPLEINFO);<br>
> break;<br>
> <br>
> + case SHADER_OPCODE_IMAGE_SIZE_LOGICAL:<br>
> + /* Nothing needs to be done except changing the opcode and setting a<br>
> + * message length.<br>
> + */<br>
> + inst->opcode = SHADER_OPCODE_IMAGE_SIZE;<br>
> + inst->mlen = inst->exec_size / 8;<br>
<br>
I'm not sure it's even worth having a LOGICAL opcode for this...<br>
maybe just set the message length when creating it and call it a day?<br></blockquote><div><br></div><div>The problem is that it has to go through SIMD width lowering which doesn't understand mlen. We could make SIMD width lowering just shrink the mlen but that's wrong for almost all opcodes. Thoughts?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + break;<br>
> +<br>
> case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL:<br>
> lower_surface_logical_send(ibld, inst,<br>
> SHADER_OPCODE_UNTYPED_SURFACE_READ,<br>
> diff --git a/src/intel/compiler/brw_fs_generator.cpp b/src/intel/compiler/brw_fs_generator.cpp<br>
> index e265d59ccbe..13293d7efbd 100644<br>
> --- a/src/intel/compiler/brw_fs_generator.cpp<br>
> +++ b/src/intel/compiler/brw_fs_generator.cpp<br>
> @@ -1017,6 +1017,9 @@ fs_generator::generate_tex(fs_inst *inst, struct brw_reg dst, struct brw_reg src<br>
> case SHADER_OPCODE_SAMPLEINFO:<br>
> msg_type = GEN6_SAMPLER_MESSAGE_SAMPLE_SAMPLEINFO;<br>
> break;<br>
> + case SHADER_OPCODE_IMAGE_SIZE:<br>
> + msg_type = GEN5_SAMPLER_MESSAGE_SAMPLE_RESINFO;<br>
> + break;<br>
<br>
Indentation here seems off.<br></blockquote><div><br></div><div>Wow. Somehow I got tabs in there. My .vimrc should make that almost impossible...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Maybe just add this case label above the<br>
existing case SHADER_OPCODE_TXS, which does the exact same thing?<br></blockquote><div><br></div><div>Sure, why not.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> default:<br>
> unreachable("not reached");<br>
> }<br>
> @@ -1126,10 +1129,20 @@ fs_generator::generate_tex(fs_inst *inst, struct brw_reg dst, struct brw_reg src<br>
> }<br>
> }<br>
> <br>
> - uint32_t base_binding_table_index = (inst->opcode == SHADER_OPCODE_TG4 ||<br>
> - inst->opcode == SHADER_OPCODE_TG4_OFFSET)<br>
> - ? prog_data->binding_table.gather_texture_start<br>
> - : prog_data->binding_table.texture_start;<br>
> + uint32_t base_binding_table_index;<br>
> + switch (inst->opcode) {<br>
> + case SHADER_OPCODE_TG4:<br>
> + case SHADER_OPCODE_TG4_OFFSET:<br>
> + base_binding_table_index = prog_data->binding_table.gather_texture_start;<br>
> + break;<br>
> + case SHADER_OPCODE_IMAGE_SIZE:<br>
> + /* Image binding table indices are absolute */<br>
<br>
Uh, what? That doesn't seem right. Shouldn't this be<br>
<br>
base_binding_table_index = prog_data->binding_table.image_start;<br></blockquote><div><br></div><div>No... It's dumb. The binding table index that gets placed in the uniform image param is an absolute binding table index. If I move this patch after the second-to-last patch, we should be able to avoid this mess. Why don't I try to do that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + base_binding_table_index = 0;<br>
> + break;<br>
> + default:<br>
> + base_binding_table_index = prog_data->binding_table.texture_start;<br>
> + break;<br>
> + }<br>
> <br>
> if (surface_index.file == BRW_IMMEDIATE_VALUE &&<br>
> sampler_index.file == BRW_IMMEDIATE_VALUE) {<br>
> @@ -2114,6 +2127,11 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)<br>
> case SHADER_OPCODE_SAMPLEINFO:<br>
> generate_tex(inst, dst, src[0], src[1], src[2]);<br>
> break;<br>
> +<br>
> + case SHADER_OPCODE_IMAGE_SIZE:<br>
> + generate_tex(inst, dst, src[0], src[1], brw_imm_ud(0));<br>
> + break;<br>
> +<br>
<br>
Indentation seems off here too.<br></blockquote><div><br></div><div>Yup. Tabs again.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> case FS_OPCODE_DDX_COARSE:<br>
> case FS_OPCODE_DDX_FINE:<br>
> generate_ddx(inst, dst, src[0]);<br>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp<br>
> index 021a31d069c..ac45b559830 100644<br>
> --- a/src/intel/compiler/brw_fs_nir.cpp<br>
> +++ b/src/intel/compiler/brw_fs_nir.cpp<br>
> @@ -3914,6 +3914,32 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr<br>
> break;<br>
> }<br>
> <br>
> + case nir_intrinsic_image_deref_size: {<br>
> + nir_deref_instr *deref = nir_src_as_deref(instr->src[0]);<br>
> + const fs_reg image = get_nir_image_deref(deref);<br>
> +<br>
> + /* The LOD also serves as the message payload */<br>
> + fs_reg lod = bld.vgrf(BRW_REGISTER_TYPE_UD);<br>
> + bld.MOV(lod, brw_imm_ud(0));<br>
> +<br>
> + fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_UD, 4);<br>
> + fs_inst *inst = bld.emit(SHADER_OPCODE_IMAGE_SIZE_LOGICAL,<br>
> + tmp, lod, image);<br>
> + inst->size_written = type_sz(tmp.type) * bld.dispatch_width() * 4;<br>
> +<br>
> + for (unsigned c = 0; c < instr->dest.ssa.num_components; ++c) {<br>
> + if (c == 2 && glsl_get_sampler_dim(deref->type) == GLSL_SAMPLER_DIM_CUBE) {<br>
> + bld.emit(SHADER_OPCODE_INT_QUOTIENT,<br>
> + offset(retype(dest, tmp.type), bld, c),<br>
> + offset(tmp, bld, c), brw_imm_ud(6));<br>
> + } else {<br>
> + bld.MOV(offset(retype(dest, tmp.type), bld, c),<br>
> + offset(tmp, bld, c));<br>
> + }<br>
> + }<br>
> + break;<br>
> + }<br>
> +<br>
> case nir_intrinsic_image_deref_load_param_intel: {<br>
> nir_deref_instr *deref = nir_src_as_deref(instr->src[0]);<br>
> const fs_reg image = get_nir_image_deref(deref);<br>
> diff --git a/src/intel/compiler/brw_nir_lower_image_load_store.c b/src/intel/compiler/brw_nir_lower_image_load_store.c<br>
> index a4bd37fc1e5..99206d81ee0 100644<br>
> --- a/src/intel/compiler/brw_nir_lower_image_load_store.c<br>
> +++ b/src/intel/compiler/brw_nir_lower_image_load_store.c<br>
> @@ -727,6 +727,21 @@ lower_image_size_instr(nir_builder *b,<br>
> nir_intrinsic_instr *intrin)<br>
> {<br>
> nir_deref_instr *deref = nir_src_as_deref(intrin->src[0]);<br>
> + nir_variable *var = nir_deref_instr_get_variable(deref);<br>
> +<br>
> + /* For write-only images, we have an actual image surface so we fall back<br>
> + * and let the back-end emit a TXQ for this.<br>
<br>
We use "TXS" for textureSize(). TXQ is a TGSI name. Please s/TXQ/TXS/g<br></blockquote><div><br></div><div>Fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + */<br>
> + if (var->data.image.write_only)<br>
> + return false;<br>
> +<br>
> + /* If we have a matching typed format, then we have an actual image surface<br>
> + * so we fall back and let the back-end emit a TXQ for this.<br></blockquote><div><br></div><div>Here too.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + */<br>
> + const enum isl_format image_fmt =<br>
> + isl_format_for_gl_format(var->data.image.format);<br>
> + if (isl_has_matching_typed_storage_image_format(devinfo, image_fmt))<br>
> + return false;<br>
> <br>
> b->cursor = nir_instr_remove(&intrin->instr);<br>
> <br>
> diff --git a/src/intel/compiler/brw_shader.cpp b/src/intel/compiler/brw_shader.cpp<br>
> index 804573fc631..c517451cd2d 100644<br>
> --- a/src/intel/compiler/brw_shader.cpp<br>
> +++ b/src/intel/compiler/brw_shader.cpp<br>
> @@ -267,6 +267,11 @@ brw_instruction_name(const struct gen_device_info *devinfo, enum opcode op)<br>
> case SHADER_OPCODE_SAMPLEINFO_LOGICAL:<br>
> return "sampleinfo_logical";<br>
> <br>
> + case SHADER_OPCODE_IMAGE_SIZE:<br>
> + return "image_size";<br>
> + case SHADER_OPCODE_IMAGE_SIZE_LOGICAL:<br>
> + return "image_size_logical";<br>
> +<br>
> case SHADER_OPCODE_SHADER_TIME_ADD:<br>
> return "shader_time_add";<br>
> <br>
> <br>
<br>
</blockquote></div></div>