<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Jun 3, 2018 at 4:18 PM, Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sat, Jun 2, 2018 at 2:48 AM, Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> wrote: > On Fri, Jun 1, 2018 at 1:01 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: >> This is something that Connor and I have been talking about for some time >> now. The basic idea is to replace the current singly linked nir_deref list >> with deref instructions. This is similar to what LLVM does and it offers >> quite a bit more freedom when we start getting more realistic pointers from >> compute applications. >> >> Right now, we still have a fully "logical" pointer model where pointer >> chains eventually terminate at variable dereferences. In future, we'd like >> to be able to use nir_deref_instrs for things like UBOs and SSBOs where tha >> tmay not be the case. There are still a couple open questions around how >> we want to handle "raw" pointers in SPIR-V for OpenCL drivers particularly >> around casting. However, the hard part is getting the deref instructions >> and getting everything switched over to them. Now that we've done that, >> some of those other details can be sorted out later. >> >> This series is, as far as Rob, Bas, and I can tell, a complete and correct >> transition for all NIR-based drivers. The final patch deletes the data >> structures and helpers for the older deref chains. >> >> A massive thank you goes out to Rob for putting the final patch set >> together and trying to get things in an order that will hopefully not >> regress anyone. Thanks also to Bas for fixing up the radeon and radv >> bits. >> >> This series can be found as a branch on gitlab: >> >> <a href="https://gitlab.freedesktop.org/jekstrand/mesa/commits/review/nir-deref-instr-v4" rel="noreferrer" target="_blank">https://gitlab.freedesktop.org/jekstrand/mesa/commits/review/nir-deref-instr-v4</a> >> >> Ideally, I'd like the series to get some amount of real review before it >> lands. Honestly, it's been baking long enough and tested by enough people >> on enough drivers that we can probably throw a bunch of Acked-by and >> Tested-bys on it and call it a day but I'd rather not. I plan to review >> all of the patches I didn't write but that will have to wait until >> tomorrow. >> >> At the very least, I'd like some sort of an ACK from a variety of the >> people that use NIR on the core concept and the sort of general shape of >> things at the end of the series. A lot of work has gone into this but it's >> also a big change and the more positive feedback it gets, the more >> comfortable I'll be pulling the trigger. > > fwiw, with the issues that Bas hit, strong a-b from me.. </div></div>With the fixups I just sent (and which the list will hopefully pick up soon ...) and the mentioned patch moves in the cover letter (and Daves r-b applied on the original AMD patches?), this series is acked-by me. </blockquote><div> </div><div>Cool. I've applied your patches and they are now in my wip/nir-deref-instrs branch. Please double-check that I applied them correctly.</div><div> </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - Bas <div class="HOEnZb"><div class="h5"> > > I've been working with the patchset for a while, (and a big stack of > compute related patches on top that I'd like to be able to start > sending to list) and the handful of comments made on irc in the early > stages have been addressed. This makes for a much cleaner base to > start adding "real" pointer support for compute, so I'm totally happy, > it works out much cleaner than earlier attempts based on working > around deref chains :-) > > not a traditional patch-by-patch review, so not really sure r-b is > appropriate, but I'm pretty happy with the result (and the amount of > churn involved does make tranditional patch-by-patch review difficult) > > BR, > -R > >> >> Thanks, >> >> --Jason Ekstrand >> >> >> >> Cc: Rob Clark <<a href="mailto:robdclark@gmail.com">robdclark@gmail.com</a>> >> Cc: Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> >> Cc: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> >> Cc: Connor Abbott <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> >> Cc: Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl">bas@basnieuwenhuizen.nl</a>> >> Cc: Karol Herbst <<a href="mailto:kherbst@redhat.com">kherbst@redhat.com</a>> >> Cc: Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> >> Cc: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> >> Cc: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> >> >> Bas Nieuwenhuizen (16): >> ac/nir: Implement the deref instr for shared memory. >> ac/nir: Support deref instructions in get_sampler_desc. >> ac/nir: Support deref instructions in tex instructions. >> ac/nir: Implement derefs for integer gather4 lowering. >> ac/nir: Add deref support to image intrinsics. >> radv: Add shader info support for image deref instructions. >> ac/nir: Add deref based var loads/stores. >> radv: Gather info for deref instr based load/store. >> ac/nir: Add shared atomic deref instr support. >> ac/nir: Add deref interp support. >> radv: Use deref instructions for tex derefs in meta shaders. >> radv: Remove image_var stores. >> radeonsi: Add deref support to the nir scan pass. >> ac/nir: Remove deref chain support. >> radv: Remove deref chain support in radv shader info pass. >> radeonsi: Remove deref chain support in nir scan pass. >> >> Eric Anholt (1): >> broadcom/vc4: Remove deref chain support from nir_lower_txf_ms. >> >> Jason Ekstrand (98): >> nir/validate: Rework intrinsic type validation >> nir: Add a deref instruction type >> nir/builder: Add deref building helpers >> nir: Add _deref versions of all of the _var intrinsics >> nir: Add deref sources to texture instructions >> nir: Add helpers for working with deref instructions >> nir/deref: Add some deref cleanup functions >> anv,i965,radv,st,ir3: Call nir_lower_deref_instrs >> glsl/nir: Only claim to handle intrinsic functions >> glsl/nir: Use deref instructions instead of dref chains >> prog/nir: Use deref instructions for params >> nir: Support deref instructions in remove_dead_variables >> nir: Add a pass for fixing deref modes >> nir: Support deref instructions in lower_global_vars_to_local >> nir: Support deref instructions in lower_io_to_temporaries >> nir: Add a deref path helper struct >> nir: Support deref instructions in lower_var_copies >> nir: Support deref instructions in split_var_copies >> nir: Support deref instructions in lower_vars_to_ssa >> nir: Support deref instructions in lower_indirect_derefs >> nir: Support deref instructions in lower_system_values >> nir: Support deref instructions in lower_clip_cull >> nir: Support deref instructions in propagate_invariant >> nir: Support deref instructions in gather_info >> nir: Support deref instructions in lower_io >> nir: Support deref instructions in lower_atomics >> nir: Support deref instructions in lower_wpos_ytransform >> nir: Support deref instructions in lower_pos_center >> nir: Support deref instructions in remove_unused_varyings >> nir: Support deref instructions in loop_analyze >> nir: Support deref instructions in lower_alpha_test >> nir: Support deref instructions in lower_clamp_color_outputs >> nir: Support deref instructions in lower_drawpixels >> nir: Consider deref instructions in lower_phis_to_scalar >> nir: Consider deref instructions in opt_peephole_select >> nir: Support deref instructions in opt_undef >> intel,ir3: Disable nir_opt_copy_prop_vars >> intel/nir: Fixup deref modes after lowering patch vertices >> nir/lower_tex: Always copy deref and offset sources >> i965: Move nir_lower_deref_instrs to right before locals_to_regs >> st/nir: Move lower_deref_instrs later >> spirv: Use deref instructions for most variables >> nir: Add a concept of per-member structs and a lowering pass >> nir/spirv: Pass nir_variable_data into apply_var_decoration >> spirv: Use NIR per-member splitting >> spirv: Make push constants an offset-based pointer >> spirv: Clean up vtn_pointer_to_offset >> spirv: Allow pointers to have a deref at the base >> spirv: Update vtn_pointer_to/from_ssa to handle deref pointers >> spirv: Record the type of functions >> spirv/cfg: Make the builder fully capable for both walks >> nir,spirv: Rework function calls >> anv/pipeline: Do less deref instruction lowering >> anv/pipeline: Convert lower_input_attachments to deref instructions >> anv/pipeline: Convert YCbCr lowering to deref instructiosn >> anv/pipeline: Convert lower_multiview to deref instructions >> anv/apply_pipeline_layout: Simplify extract_tex_src_plane >> anv/pipeline: Convert apply_pipeline_layout to deref instructions >> intel/fs: Use image_deref intrinsics instead of image_var >> intel/nir: Only lower load/store derefs >> intel/blorp: Stop setting tex->texture/sampler >> nir/lower_samplers: Clean up function arguments >> nir: Use derefs in nir_lower_samplers >> nir/builder: Use deref instructions for load/store/copy_var >> nir: Use deref instructions in lower_constant_initializers >> nir/vars_to_ssa: Add an is_direct field to deref_node >> nir/vars_to_ssa: Rework to entirely use deref instructions >> nir: Rework gather_info to entirely use deref instructions >> nir: Remove deref chain support from lower_indirect_derefs >> nir: Remove deref chain support from lower_clip_cull_distance_arrays >> nir: Remove deref chain support from lower_atomics >> nir: Remove deref chain support from lower_alpha_test >> nir: Remove deref chain support from lower_clamp_color_outputs >> nir: Remove deref chain support from lower_global_vars_to_local >> nir/lower_io: Convert atomic lowering to deref instructions >> nir: Convert lower_io to deref instructions >> nir: Remove deref chain support from lower_phis_to_scalar >> nir: Delete lower_io_types >> nir: Remove deref chain support from remove_unused_varyings >> nir: Remove deref chain support from lower_system_values >> nir: Remove deref chain support from lower_wpos_center >> nir: Remove deref chain support from lower_wpos_ytransform >> nir: Remove deref chain support from lower_tex >> nir: Remove deref chain support from opt_peephole_select >> nir: Remove deref chain support from lower_drawpixels >> nir: Remove deref chain support from lower_var_copies >> nir: Remove deref chain support from propagate_invariant >> nir: Remove deref chain support from dead_variables >> nir: Remove deref chain support from split_var_copies >> nir: Remove deref chain support from opt_undef >> nir: Remove deref chain support from split_per_member_structs >> nir/copy_prop_vars: Re-order some logic in compare_derefs >> nir: Rework opt_copy_prop_vars to use deref instructions >> intel,ir3: Re-enable nir_opt_copy_prop_vars >> nir: Rework lower_locals_to_regs to use deref instructions >> nir: Remove deref chain support from analyze_loops >> nir: Remove old-school deref chain support >> nir/lower_system_values: Assert/assume direct var derefs >> >> Rob Clark (14): >> nir: add deref lowering sanity checking >> ttn: convert to deref instructions >> nir/lower_samplers: split out _legacy version for deref chains >> move lower_deref_instrs >> nir: convert lower_io_to_scalar to deref instructions >> mesa/st: temporarily disable lower_io_to_elements() >> mesa/st/nir: convert lower_builtins to deref instructions >> nir: convert lower_io_arrays_to_elements to deref instructions >> mesa/st: re-enable lower_io_to_elements() >> nir: convert lower_samplers_as_deref to deref instructions >> nir/lower_samplers: remove legacy version >> st,ir3,radeonsi: push lower_deref_instrs back into driver >> nir: promote intrinsic_get_var() to helper >> freedreno/ir3: convert to deref instructions >> >> src/amd/common/ac_nir_to_llvm.c | 547 ++++++++++---------- >> src/amd/vulkan/radv_meta.c | 20 +- >> src/amd/vulkan/radv_meta_blit.c | 30 +- >> src/amd/vulkan/radv_meta_blit2d.c | 21 +- >> src/amd/vulkan/radv_meta_bufimage.c | 62 +-- >> src/amd/vulkan/radv_meta_fast_clear.c | 17 +- >> src/amd/vulkan/radv_meta_resolve_cs.c | 10 +- >> src/amd/vulkan/radv_shader.c | 8 + >> src/amd/vulkan/radv_shader_info.c | 139 ++--- >> src/compiler/Makefile.sources | 4 +- >> src/compiler/glsl/gl_nir_lower_atomics.c | 137 ++--- >> src/compiler/glsl/gl_nir_lower_samplers.c | 162 +++--- >> src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 180 ++++--- >> src/compiler/glsl/glsl_to_nir.cpp | 267 ++++------ >> src/compiler/nir/meson.build | 4 +- >> src/compiler/nir/nir.c | 423 +++------------- >> src/compiler/nir/nir.h | 207 ++++---- >> src/compiler/nir/nir_builder.h | 254 ++++++++-- >> src/compiler/nir/nir_clone.c | 143 ++---- >> src/compiler/nir/nir_deref.c | 165 ++++++ >> src/compiler/nir/nir_deref.h | 55 ++ >> src/compiler/nir/nir_gather_info.c | 51 +- >> src/compiler/nir/nir_inline_functions.c | 193 ++----- >> src/compiler/nir/nir_instr_set.c | 101 +++- >> src/compiler/nir/nir_intrinsics.py | 99 ++-- >> src/compiler/nir/nir_intrinsics_c.py | 1 - >> src/compiler/nir/nir_linking_helpers.c | 47 +- >> src/compiler/nir/nir_loop_analyze.c | 70 ++- >> src/compiler/nir/nir_lower_alpha_test.c | 7 +- >> src/compiler/nir/nir_lower_clamp_color_outputs.c | 9 +- >> .../nir/nir_lower_clip_cull_distance_arrays.c | 85 ++-- >> src/compiler/nir/nir_lower_constant_initializers.c | 57 ++- >> src/compiler/nir/nir_lower_drawpixels.c | 10 +- >> src/compiler/nir/nir_lower_global_vars_to_local.c | 45 +- >> src/compiler/nir/nir_lower_indirect_derefs.c | 174 +++---- >> src/compiler/nir/nir_lower_io.c | 185 ++++--- >> src/compiler/nir/nir_lower_io_arrays_to_elements.c | 154 +++--- >> src/compiler/nir/nir_lower_io_to_scalar.c | 90 ++-- >> src/compiler/nir/nir_lower_io_to_temporaries.c | 2 + >> src/compiler/nir/nir_lower_io_types.c | 176 ------- >> src/compiler/nir/nir_lower_locals_to_regs.c | 186 ++++--- >> src/compiler/nir/nir_lower_phis_to_scalar.c | 14 +- >> src/compiler/nir/nir_lower_system_values.c | 31 +- >> src/compiler/nir/nir_lower_tex.c | 47 +- >> src/compiler/nir/nir_lower_var_copies.c | 165 +++--- >> src/compiler/nir/nir_lower_vars_to_ssa.c | 330 ++++++------ >> src/compiler/nir/nir_lower_wpos_center.c | 8 +- >> src/compiler/nir/nir_lower_wpos_ytransform.c | 33 +- >> src/compiler/nir/nir_opt_constant_folding.c | 53 -- >> src/compiler/nir/nir_opt_copy_prop_vars.c | 316 ++++++------ >> src/compiler/nir/nir_opt_copy_propagate.c | 95 ++-- >> src/compiler/nir/nir_opt_dce.c | 7 + >> src/compiler/nir/nir_opt_peephole_select.c | 4 +- >> src/compiler/nir/nir_opt_undef.c | 2 +- >> src/compiler/nir/nir_print.c | 252 +++++---- >> src/compiler/nir/nir_propagate_invariant.c | 17 +- >> src/compiler/nir/nir_remove_dead_variables.c | 152 +++--- >> src/compiler/nir/nir_serialize.c | 234 ++++----- >> src/compiler/nir/nir_split_per_member_structs.c | 208 ++++++++ >> src/compiler/nir/nir_split_var_copies.c | 234 ++------- >> src/compiler/nir/nir_sweep.c | 4 - >> src/compiler/nir/nir_validate.c | 247 ++++----- >> src/compiler/spirv/spirv_to_nir.c | 184 ++++--- >> src/compiler/spirv/vtn_cfg.c | 231 ++++----- >> src/compiler/spirv/vtn_glsl450.c | 19 +- >> src/compiler/spirv/vtn_private.h | 26 +- >> src/compiler/spirv/vtn_variables.c | 563 +++++++-------------- >> src/gallium/auxiliary/nir/tgsi_to_nir.c | 52 +- >> src/gallium/drivers/freedreno/ir3/ir3_cmdline.c | 1 - >> .../drivers/freedreno/ir3/ir3_compiler_nir.c | 49 +- >> src/gallium/drivers/freedreno/ir3/ir3_nir.c | 4 +- >> .../freedreno/ir3/ir3_nir_lower_tg4_to_tex.c | 4 +- >> src/gallium/drivers/radeonsi/si_shader_nir.c | 72 ++- >> src/gallium/drivers/vc4/vc4_nir_lower_txf_ms.c | 1 - >> src/intel/blorp/blorp_blit.c | 2 - >> src/intel/compiler/brw_fs.h | 2 +- >> src/intel/compiler/brw_fs_nir.cpp | 157 +++--- >> src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 187 ++++--- >> src/intel/vulkan/anv_nir_lower_input_attachments.c | 31 +- >> src/intel/vulkan/anv_nir_lower_multiview.c | 17 +- >> src/intel/vulkan/anv_nir_lower_ycbcr_textures.c | 34 +- >> src/intel/vulkan/anv_pipeline.c | 7 + >> src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 2 + >> src/mesa/drivers/dri/i965/brw_program.c | 5 +- >> src/mesa/program/prog_to_nir.c | 41 +- >> src/mesa/state_tracker/st_nir_lower_builtin.c | 62 ++- >> 86 files changed, 4183 insertions(+), 4620 deletions(-) >> create mode 100644 src/compiler/nir/nir_deref.c >> create mode 100644 src/compiler/nir/nir_deref.h >> delete mode 100644 src/compiler/nir/nir_lower_io_types.c >> create mode 100644 src/compiler/nir/nir_split_per_member_structs.c >> >> -- >> 2.5.0.400.gff86faf >> </div></div></blockquote></div> </div></div>