This is one step towards lowering I/O during shader preprocess rather than at
variant create time, which helps mitigate shader variant jank. It's also a lot
simpler.
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Alyssa Rosenzweig <alyssa@collabora.com>
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com> [v1]
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20836>
In OpenGL, FRAG_RESULT_COLOR implicitly broadcasts to every render target. Our
existing lower_blend code (somewhat arbitrarily) aliases to the the first render
target's format and blend settings. That said, I don't think that works if
different render targets have different settings -- or blend with their
different destinations -- though I don't have relevant spec text right now.
The actual reason this works is that all users of this pass either call
nir_lower_fragcolor first (panfrost, asahi) or don't have FRAG_RESULT_COLOR as
part of their API (panvk, soon agxv). Unless/until we actually have a use case
for nir_lower_blend with gl_FragColor, assert that gl_FragColor is lowered first
so we don't need to worry about this imaginary case.
Signed-off-by: Alyssa Rosenzweig <alyssa@collabora.com>
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20836>
Stores don't have destinations, and if they did, it would be invalid to change
their ssa_def's num_components without also changing the SSA def. Remove the
nonsensical (but harmless) assignment.
This fixes 25249e8be2 ("nir/lower_blend: Expand or shrink output variables as
needed"), but as the bug is harmless in practice, it does not need to be
backported.
Signed-off-by: Alyssa Rosenzweig <alyssa@collabora.com>
Suggested-by: Timur Kristóf <timur.kristof@gmail.com>
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Reviewed-by: Italo Nicola <italonicola@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20836>
This is a form of lowered I/O, it needs I/O semantics so we can know the
location to store to instead of passing via a sideband.
Over in !20906, we will use the BASE to lower blend shader with multisampling in
NIR instead of passing the number of samples and framebuffer format along a
sideband to the Midgard compiler. That's not needed for this series (this patch
was cherry-picked to avoid regressions in the lower_blend changes) but it's good
to model the full form of the I/O lowered intrinsic here.
Signed-off-by: Alyssa Rosenzweig <alyssa@collabora.com>
Reviewed-by: Italo Nicola <italonicola@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20836>
No longer used. All of the information that was previously track here is
tracked directly in nir_loop_variable... and, technically speaking, has
been tracked there ever since 0b9639c35d.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
These track the same information in a slightly different way. Since
nir_loop_variable::init_src is visible outside this module, it cannot
be eliminated.
As an intentional side effect, induction variables with constant
initializers will now have their nir_loop_induction_variable::init_src
field point to the load_const source. Previously this pointer would be
NULL.
v2: Update unit tests and commit message. Remove the now unused ind_var
variable in find_trip_count.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
These track the same information in a slightly different way. Since
nir_loop_variable::update_src is visible outside this module, it cannot
be eliminated.
This leads to some nice simplification in find_trip_count. Previously
this code only had access to the ALU instruction that performs the
increment. It had to "search" the parameters to determine which (if any)
was the constant. With this change, this code has access to the
nir_alu_src of the ALU instruction that performs the increment. It no
longer needs to search the parameters for the constant. It's either the
supplied nir_alu_src or nothing.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
As an intentional side effect, induction variables with constant
increments will now have their nir_loop_induction_variable::update_src
field point to the load_const source. Previously this pointer would be
NULL.
v2: Update unit tests and commit message.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
A couple basic tests for loops with the exit condition after the
increment. In compiler literature, the optimization that moves the exit
condition from the top to the bottom is called "loop inversion."
v2: Pass parameters to loop_builder_invert using a struct. Add a comment
describing the loop being constructed to loop_builder_invert. Both
suggested by Caio.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
Inspired heavily by the work by Yevhenii Kolesnikov in the original
versions of !3445.
v2: Pass parameters to loop_builder using a struct. Add a comment
describing the loop being constructed to loop_builder. Both suggested by
Caio.
v3: mscv C++ designated initializer lolz.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
All of the other tests only log the shader when validation fails, so
having that shader scroll by in the output is very distracting.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21289>
This shader originates from Granite 3D engine and has been adapted
to be used with Open GL and some GLSL ES specifics.
GLSL ES adaptation:
- remove Vulkan specifics: EXT_samplerless_texture_functions usage,
specialization constants, push constant usage
- inline bitextract.h
- always DECODE_8BIT and hardcode error color (for now)
- port to GLSL ES, required some type changes, explicit type
conversions and setting up precisions for types
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Nanley Chery <nanley.g.chery@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/19886>
This is based on brw_nir_lower_mem_access_bit_sizes() but ended up being
substantially different. While the core concepts are all the same, the
brw_* version made a lot of Intel-specific assumptions. The new version
takes a callback which takes a number of bytes of data and an alignment
pair and returns a bit size and number of components to load/store.
Reviewed-by: M Henning <drawoc@darkrefraction.com>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21232>
This makes the pass significantly faster cutting execution time
by around 30% in the cts test
dEQP-GLES31.functional.ubo.random.all_per_block_buffers.20
This 30% improvement is in addition to all the improvements from
the proceeding patches.
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20381>
As per the previous commit if we don't reuse these dynamic arrays
we end up needlessly thrashing the memory handling functions.
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20381>
Due to how this pass works we can end up thrashing memory if we
do not reuse these hash tables rather than reusing them.
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20381>
Previously the pass was comparing every deref to every load/store
causing the pass to slow down more the larger the shader is.
Here we use a hash table so we can simple store everything needed
for comparision of a var separately.
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20381>
The fix in 947f7b452a introduced an extra loop over the copies
array to find the correct entry in the case it had been moved.
The problem is these loops can be iterated over millions of times
so lets simply update the entry pointer in the case we change its
location in the array.
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20381>
We loop, effectively, over two stacks: ready and to_do and finish only
when both are empty. In the case where ready is empty, we pull one off
of to_do, add a copy to a temporary, and push it onto the ready stack.
Previously, we assumed that we would never get to the temporary copy
case if to_do has exactly one entry because that would imply that there
was only one copy left which means there can't possibly be a cycle to
break. This was true until c7fc44f9eb ("nir/from_ssa: Respect and
populate divergence information") which changed things such that
temporary copies sometimes get added in the case where a convergent
value is copied both to convergent and divergent destinations.
This patch adjusts our loop iteration to always attempt to clear the
ready stack before checking if there's anything left on the to_do stack.
I also added an assert to make the exit condition more clear.
Fixes: c7fc44f9eb ("nir/from_ssa: Respect and populate divergence information")
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/8037
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Daniel Schürmann <daniel@schuermann.dev>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21315>
There is an optimization in the parallel copy algorithm where, after a
copy has been performed, we can treat the destination as the new source
for future copies of the same source. In particular, consider the
following parallel copy: A -> B, C -> A, A -> C. In this case, after we
have done the A -> B copy, we can make note that the value in A is now
in B and emit the sequence: A -> B, C -> A, B -> C. This allows us to
resolve the swap cycle between A anc C without allocating a temporary
register because we know B is also a copy of A.
When one of the registers involved is convergent and the other is
divergent, this optimization is problematic because, while convergent to
divergent copies are fine, we can't re-use the divergent copy in later
copies if any of those copies are to a convergent variable. We could,
but it would require a read_first_invocation which would get messy. In
In c7fc44f9eb ("nir/from_ssa: Respect and populate divergence
information"), we attempted to deal with this by limiting the rename
optimization to the case where the divergence matched.
The problem is that we did the re-name part whenever the divergence
matched but only marked it as ready if the thing being copied was a
destination. (We actually left two instances of loc[a] = b, one which
always happened and one which only happened if we also wanted to flag
the source as being ready to use as a destination.) While this
technically doesn't cause any problems, it may result in more inter-mov
dependencies which hurts instruction scheduling. For example, if we had
the parallel copy A -> B, A -> C, A -> D, we now end up emitting the
sequence A -> B, B -> C, C -> D which has many more data hazards between
instructions caused by the constant shuffling.
This commit restores the original logic in which we only perform the
rename optimization if the rename would free up a register we will later
use as a destination. This isn't entirely optimal as it still doesn't
prove that there is a cycle involved first, but it should lead to a
reduction in unnecessary dependencies.
No shader-db changes on SKL or DG2
Fixes: c7fc44f9eb ("nir/from_ssa: Respect and populate divergence information")
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21315>
This is commit 4397c166c0 for spirv_to_nir, otherwise we hit
the same assert with anv driver.
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Konstantin Seurer <konstantin.seurer@gmail.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21281>
This patch causes us to print "con" and "div" for registers as well as
SSA defs. We print it on both register declarations, and destinations.
The latter isn't strictly necessary, but it is handy to be able to see
e.g. a convergent value being assigned to a divergent register without
having to constantly refer back to definitions that might be much
earlier in the program. I originally printed it for sources as well,
but that got to be a bit wordy, so I dropped that.
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21299>
Pointed out by GCC:
In function ‘load_text_file’,
inlined from ‘standalone_compile_shader’ at ../src/compiler/glsl/standalone.cpp:491:38,
inlined from ‘main’ at ../src/compiler/glsl/main.cpp:98:45:
../src/compiler/glsl/standalone.cpp:358:17: error: ‘free’ called on pointer ‘block_195’ with nonzero offset 48 [-Werror=free-nonheap-object]
358 | free(text);
| ^
In function ‘ralloc_size’,
inlined from ‘load_text_file’ at ../src/compiler/glsl/standalone.cpp:352:31,
inlined from ‘standalone_compile_shader’ at ../src/compiler/glsl/standalone.cpp:491:38,
inlined from ‘main’ at ../src/compiler/glsl/main.cpp:98:45:
../src/util/ralloc.c:117:18: note: returned from ‘malloc’
117 | void *block = malloc(align64(size + sizeof(ralloc_header),
| ^
Fixes: a9696e79fb ("main: Close memory leak of shader string from load_text_file.")
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21215>
Drop the unused ctx parameter, to match the main Mesa code.
Fixes ODR violation flagged by -Wodr with LTO enabled:
../src/mesa/main/shaderobj.h:74:1: error: ‘_mesa_reference_shader_program_data’ violates the C++ One Definition Rule [-Werror=odr]
74 | _mesa_reference_shader_program_data(struct gl_shader_program_data **ptr,
| ^
../src/compiler/glsl/standalone_scaffolding.cpp:76:1: note: type mismatch in parameter 1
76 | _mesa_reference_shader_program_data(struct gl_context *ctx,
| ^
../src/compiler/glsl/standalone_scaffolding.cpp:76:1: note: ‘_mesa_reference_shader_program_data’ was previously declared here
../src/compiler/glsl/standalone_scaffolding.cpp:76:1: note: code may be misoptimized unless ‘-fno-strict-aliasing’ is used
Fixes: 717a720e9c ("mesa: drop unused context parameter to shader program data reference.")
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21215>
A stackless (or at least using allocated memory for stack) version
might be nice but for now this works around some games compiling
large shaders and hitting stack overflows.
CC: mesa-stable
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21231>
This also removes the loop so opt_remove_cast_cast() will only optimize
cast(cast(x)) and not cast(cast(cast(x))). However, since nir_opt_deref
walks instructions top-down, there will almost never be a tripple cast
because the parent cast will have opt_remove_cast_cast() run on it.
Reviewed-by: Jesse Natalie <jenatali@microsoft.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21252>
While this change helps with few shaders, the main benefit is
that it allows to unroll loops comming from nine+ttn on vec4
backends. D3D9 REP ... ENDREP type loops are unrolled now already,
LOOP ... ENDLOOP need some nine changes that will come later.
r300 RV530 shader-db:
total instructions in shared programs: 132481 -> 132344 (-0.10%)
instructions in affected programs: 3532 -> 3395 (-3.88%)
helped: 13
HURT: 0
total temps in shared programs: 16961 -> 16957 (-0.02%)
temps in affected programs: 88 -> 84 (-4.55%)
helped: 4
HURT: 0
Reviewed-by: Emma Anholt <emma@anholt.net>
Signed-off-by: Pavel Ondračka <pavel.ondracka@gmail.com>
Partial fix for: https://gitlab.freedesktop.org/mesa/mesa/-/issues/8102
Partial fix for: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7222
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21038>
This invalid memory access is a consequence of wrong assumptions,
for instance:
"prog->sh.data is NULL if it's ARB_fragment_program"
This issue is triggered with piglit/fp-formats -auto -fbo:
==9747==ERROR: AddressSanitizer: heap-use-after-free on address 0x007f7c812d90 at pc 0x007f833c09f8 bp 0x007fd7eca750 sp 0x007fd7eca768
READ of size 4 at 0x007f7c812d90 thread T0
#0 0x7f833c09f4 in st_get_sampler_views ../src/mesa/state_tracker/st_atom_texture.c:109
#1 0x7f833c0b48 in update_textures ../src/mesa/state_tracker/st_atom_texture.c:266
#2 0x7f82b2d120 in st_validate_state ../src/mesa/state_tracker/st_util.h:128
#3 0x7f82b2d120 in prepare_draw ../src/mesa/state_tracker/st_draw.c:88
#4 0x7f82b2de64 in st_draw_gallium ../src/mesa/state_tracker/st_draw.c:141
#5 0x7f83105940 in _mesa_draw_arrays ../src/mesa/main/draw.c:1202
#6 0x7f8d5fa5cc in piglit_draw_rect_from_arrays piglit/tests/util/piglit-util-gl.c:711
#7 0x7f8d5fac34 in piglit_draw_rect_custom piglit/tests/util/piglit-util-gl.c:833
#8 0x4019e0 in piglit_display piglit/tests/shaders/fp-formats.c:67
#9 0x7f8d643fc4 in run_test piglit/tests/util/piglit-framework-gl/piglit_fbo_framework.c:52
#10 0x401624 in main piglit/tests/shaders/fp-formats.c:39
Signed-off-by: Patrick Lerda <patrick9876@free.fr>
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21175>
From the 96c19d23c9 commit message:
Ever since 4246c2869c and 7d85dc4f35 loop unrolling can no
longer depend on inot being eliminated from the loop
terminator condition so we need to be able to handle it.
Support these conditions here too.
Acked-by: Mike Blumenkrantz <michael.blumenkrantz@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21179>
While making the function public, rename it to
nir_collect_src_uniforms. The old name makes it sound like it's just a
query that doesn't have side effects. That is, however, not the case.
This is step 4 in an attempt to unify a bunch of nir_inline_uniforms.c
and lvp_inline_uniforms.c code.
Reviewed-by: Mike Blumenkrantz <michael.blumenkrantz@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21179>
Only caller in this file still only passes 1.
This is step 2 in an attempt to unify a bunch of nir_inline_uniforms.c
and lvp_inline_uniforms.c code.
Reviewed-by: Mike Blumenkrantz <michael.blumenkrantz@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21179>
max_num_bo is currently limited to 1. That will change in the next
commit.
This is step 1 in an attempt to unify a bunch of nir_inline_uniforms.c
and lvp_inline_uniforms.c code.
Reviewed-by: Mike Blumenkrantz <michael.blumenkrantz@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21179>
