Series Reviewed-by: Jordan Justen <[email protected]> Although, I think you said you might rewrite patch 13 (the thread_local_id_index patch). If you just add the small stage check I mentioned then you can add my r-b for it.
-Jordan On 2017-09-29 14:25:00, Jason Ekstrand wrote: > This little series reworks basically the entire world of push/pull params. > This refactor has been on my ToDo list ever since I hooked up push > constants in Vulkan over two years ago. Recently, thanks to some > up-and-coming Vulkan features, the pain of the old system increased enough > to finally push me over the edge to make the changes. > > Historically, we have had two arrays in brw_stage_prog_data: param and > pull_param which are arrays of pointers to a gl_constant_value union. In > state setup, we would walk the arrays, dereference each pointer, and copy > the value into our push or pull constant storage. These pointers may point > to uniform storage, ARB program parameter storage, or random bits of API > state that we've gathered up. This model has never really matched what we > want in Vulkan because we don't have a dedicated CPU memory location where > the push constant values are stored that we can know at compile time. > Instead, in Vulkan, we just store offsets into a struct anv_push_constants > in the pointers and trust that the back-end compiler will re-arrange the > array without ever dereferencing any of the pointers. > > This primary purpose of this series is to move us over to a model where the > param and pull_param arrays are just arrays of uint32_t handles which have > a meaning which is defined by the driver. This makes the contract between > compiler and driver a bit more clear and we stop passing random pointers to > API state into the back-end compiler. > > The secondary purpose of this series is to add support for "builtin" > params. These params have a well-defined meaning as per their builtin enum > name and will get filled out by the driver during state upload. Having a > well-defined meaning means that they can be added by either the front-end > or back-end compilers without having to do a lot of juggling in order to > make them work in two drivers. One example of this is the last patch which > moves the thread local id from being this weird special-cased thing to > being a normal builtin param. We still have to do a bit of special-case > work because it needs to be part of the per-thread push constants but other > than that, it now works more-or-less like any other param. > > Thirdly, I did a bunch of refactoring to try and get us to the point where > the param and pull_param arrays are, as far as practical, out parameters of > the compilation process rather than in-out parameters. In particular, we > stop trying to estimate in the driver how much space we need to allocate > for params and just allocate what we need on-demand in the various places > that set them up. > > This series can be found here: > > https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/i965-push-enums > > Jason Ekstrand (21): > i965: Move brw_upload_pull_constants to gen6_constant_state.c > i965: Add a helper for populating constant buffers > i965: Get rid of gen7_cs_state.c > intel: Rewrite the world of push/pull params > i965: Use prog->info.num_images for needs_dc computation > i965: Store image_param in brw_context instead of prog_data > anv/pipeline: Add a mem_ctx parameter to anv_pipeline_compile > anv/pipeline: Ralloc prog_data::param of the compile mem_ctx > intel/compiler: Add a flag for pull constant support > i965: Only add the wpos state reference if we lowered something > intel/compiler: Stop adding params for texture sizes > intel/compiler: Add a helper for growing the prog_data::param array > intel/cs: Grow prog_data::param on-demand for thread_local_id_index > intel/vs: Grow the param array for clip planes > anv/pipeline: Whack nir->num_uniforms to MAX_PUSH_CONSTANT_SIZE > anv/pipeline: Grow the param array for images > anv/pipeline: Refactor setup of the prog_data::param array > ralloc: Allow reparenting to a NULL context > intel: Allocate prog_data::[pull_]param deeper inside the compiler > intel/compiler: Allocate pull_param in assign_constant_locations > intel/cs: Make thread_local_id a regular builtin param > > src/intel/blorp/blorp.c | 2 +- > src/intel/compiler/brw_compiler.h | 98 ++++++++-- > src/intel/compiler/brw_fs.cpp | 78 ++++---- > src/intel/compiler/brw_fs.h | 6 +- > src/intel/compiler/brw_fs_visitor.cpp | 15 +- > src/intel/compiler/brw_nir.h | 5 +- > src/intel/compiler/brw_nir_intrinsics.c | 18 +- > src/intel/compiler/brw_vec4.cpp | 18 +- > src/intel/compiler/brw_vec4_visitor.cpp | 7 +- > src/intel/compiler/brw_vec4_vs.h | 3 - > src/intel/compiler/brw_vec4_vs_visitor.cpp | 11 +- > src/intel/vulkan/anv_cmd_buffer.c | 41 +++-- > src/intel/vulkan/anv_device.c | 1 + > src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 28 ++- > src/intel/vulkan/anv_pipeline.c | 116 ++++++------ > src/intel/vulkan/anv_pipeline_cache.c | 6 +- > src/intel/vulkan/anv_private.h | 3 + > src/mesa/drivers/dri/i965/Makefile.sources | 1 - > src/mesa/drivers/dri/i965/brw_context.h | 2 + > src/mesa/drivers/dri/i965/brw_cs.c | 29 +-- > src/mesa/drivers/dri/i965/brw_curbe.c | 12 +- > src/mesa/drivers/dri/i965/brw_gs.c | 28 +-- > src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 83 +++++---- > src/mesa/drivers/dri/i965/brw_program.c | 10 +- > src/mesa/drivers/dri/i965/brw_program.h | 23 +++ > src/mesa/drivers/dri/i965/brw_state.h | 14 +- > src/mesa/drivers/dri/i965/brw_tcs.c | 50 ++---- > src/mesa/drivers/dri/i965/brw_tes.c | 29 +-- > src/mesa/drivers/dri/i965/brw_vs.c | 30 +--- > src/mesa/drivers/dri/i965/brw_vs_surface_state.c | 63 ------- > src/mesa/drivers/dri/i965/brw_wm.c | 25 +-- > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 2 +- > src/mesa/drivers/dri/i965/gen6_constant_state.c | 218 > ++++++++++++++++++++++- > src/mesa/drivers/dri/i965/gen7_cs_state.c | 173 ------------------ > src/mesa/drivers/dri/i965/gen7_l3_state.c | 5 +- > src/mesa/drivers/dri/i965/genX_state_upload.c | 68 ++++++- > src/mesa/drivers/dri/i965/intel_screen.c | 1 + > src/util/ralloc.c | 2 +- > 38 files changed, 702 insertions(+), 622 deletions(-) > delete mode 100644 src/mesa/drivers/dri/i965/gen7_cs_state.c > > -- > 2.5.0.400.gff86faf > > _______________________________________________ > mesa-dev mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
