We currently assume that the instruction is already inserted and we are
optimizing it away, but in the use case I have where we are hoisting
instructions into a preamble and deduplicating as we go along, that
isn't the case. Move this responsibility onto the caller, which also
makes it a bit clearer what's going on and turns this into something
more similar to an actual set.
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29873>
This allows use cases where we copy over expression trees and
deduplicate as we go along. We can use the matching instruction to build
up the rest of the expression tree.
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29873>
Fix ANV and virpipe CI test fail when nir_fixup_deref_types
is used in nir_vectorize_tess_levels by later commits.
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29799>
Replace the implementation with nir_lower_array_deref_of_vec.
This will be used by compact_array=false drivers to lower indirect
tess levels array access to direct vector access too.
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29799>
812b3415 added rules for upcasts with comparisons with a variety of
types. The float & unsigned rules should be ok, but the signed integer rules are
unsound as currently implemented. This can cause end-to-end miscompiles.
I originally hit this issue while debugging a large real world OpenCL kernel. I
found the bug symptoms changed when disabling loop unrolling, which tipped me
off to a compiler bug. I've reduced it to a minimal test case. Imagine my
surprise when I find out the NIR my backend ingested was already constant folded
to be wrong.
In the minimal test case, during optimization we have NIR:
32 %6 = ....
64 %9 = i2i64 %6
64 %44 = load_const (0x0000000000000001)
1 %45 = ilt %9, %44 (0x1)
This is a simple check (int64_t)%6 < 1.
nir_opt_algebraic turns this into:
32 %6 = ...
64 %9 = i2i64 %6
64 %44 = load_const (0x0000000000000001)
64 %55 = load_const (0x0000000080000000 = 2147483648)
1 %56 = ilt %55 (0x80000000), %44 (0x1)
64 %57 = load_const (0x000000007fffffff = 2147483647)
1 %58 = ilt %57 (0x7fffffff), %44 (0x1)
32 %59 = i2i32 %44 (0x1)
1 %60 = ilt %6, %59
1 %61 = ior %58, %60
1 %62 = iand %56, %61
This pile of math constant-folds to an unconditional "false"! The problem is
%56. At first glance, INT32_MIN < 1 is true so %56 should be true. Indeed, it
should. But here's the kicker: both constants are 64-bit here, so the ilt
operation is a 64-bit comparison -- that left-hand side is INT32_MIN
zero-extended to 64-bit for the signed comparison at 64-bit. So in fact, it
evaluates to false, causing the whole expression to go false. If we're going to
do a 64-bit comparison for %56, then we need to sign-extend the bound. So we'll
just adjust the Python and be on our way, right?
Unfortunately the issue is deeper. According to the comment in the generated
nir_opt_algebraic.c file, the guilty algebraic rule is:
('ilt', ('i2i64', 'a@32'), '#b') =>
('iand', ('ilt', -2147483648, 'b'), ('ior', ('ilt', 2147483647, 'b'), ('ilt', 'a', ('i2i32', 'b'))))
From a Python perspective? That rule is correct. -2147483648 < 1 is a true
statement. Adjusting the Python rule is not the appropriate solution here, since
the issue is more fundamental and might affect other rules. The real problem is
the translation of that Python replacement tree into C, incorrectly
zero-extending -2147483648 into 0x0000000080000000 instead of sign-extending to
0xffffffff80000000.
Crawling down the rabbit hole of the generated algebraic file, we see the
constant encoded as:
{ .constant = {
{ nir_search_value_constant, 64 },
nir_type_int, { -0x80000000 /* -2147483648 */ },
} },
NIR correctly translates the negative constant to a C level negate operation of
its absolute value. This maps to the correct sign-extension...
...for all constants except for INT_MIN. Because that constant lacks a ULL
suffix, it is a 32-bit integer. And for this integer (only), negating it hits
signed integer overflow (UB!) and then we end up with an effective
zero-extension when going to 64-bit.
This patch fixes the end-to-end miscompile.
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Closes: #11402
Cc: mesa-stable
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29952>
Prevents regressions when removing input modifiers from a == 0.0.
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29467>
I see people open-coding this all over the tree and it makes nir_builder loops
really annoying. Make them slightly less annoying with a helper.
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Daniel Schürmann <daniel@schuermann.dev>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29877>
For preambles, we don't actually care which invocation we get, so we
don't have to enable helper invocations when the preamble uses "getone."
Introduce a new intrinsic with the right semantics and plumb it through.
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29914>
indirect store lowering will use if/else which changes
the control flow of the shader.
Fixes: 110887de2b ("nir: Add a new pass to lower array dereferences on vectors")
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29894>
indirect store lowering will change control flow,
so we should not preserve control flow metadate
when it's present.
Fixes: 35b8f6f40b ("nir: Add a new pass to lower array dereferences on vectors")
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Qiang Yu <yuq825@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29894>
The number of components for the second source is -1 to avoid validation of
its value. Some supported configurations will have the component count of
that matrix different than the others.
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28834>
Ensure unsigned integers are used instead of signed ones when performing
left bit shifts.
This has been detected by the Undefined Behaviour Sanitizer (UBSan).
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29772>
Undefined Behaviour Sanitizer (UBSan) detected the following when
running testing `dEQP-VK.graphicsfuzz.cov-fold-negate-min-int-value`:
`negation of -2147483648 cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself`
SPIR-V spec states that OpSNegate(0x80000000) has to return 0x80000000;
in our case, -2147483648 should be -2147483648.
While this is not causing any issue because compilers seem to be
behaving like that, it is still undefined behaviour, so it expects to be
this handled explicitly, which is the purpose of this commit.
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29772>
"Rewrite and remove" is a super common idiom in NIR passes. Let's add a helper
to make it more ergonomic.
More the point, I expect that /most/ of the time when a pass rewrites uses, they
also want to remove the parent instruction. The principle reason not to is
because it takes extra effort to add in the nir_instr_remove and nir_opt_dce
will clean up after you eventually, right? From a compile time perspective, it's
better to remove earlier to reduce the redundant processing between the pass and
the next DCE run. So ... we want to be doing *more* removes. From a UX
perspective - the way to nudge devs towards that is to make the
preferred "rewrite-and-remove" pattern more ergonomic than the "rewrite but
keep". That justifies the simple "replace" name rather than something silly like
"rewrite_uses_and_remove".
---
Something else I've wanted for a while.
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29817>
Yes, we're losing precision if this assert fails and it's wrong. It's also
necessary to implement GL in a reasonable way on Asahi. Remove the assert that
was recently added and add more comment context on the mess.
Fixes debug build regression on asahi:
dEQP-GLES3.functional.vertex_arrays.single_attribute.normalize.int.components4_quads1
Fixes: 22f1b04a99 ("nir/format_convert: Assert that UNORM formats are <= 16 bits")
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Suggested-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29820>
There's no good reason for this to be header-only besides laziness on my
part when I first wrote a few "small" helpers. Some of those are pretty
good sized and don't need to be inlined.
Keeping the original copyright since this is just moving code.
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28793>
As long as drivers implement an fmin/fmax that do the right thing with
NaN, there's no reason for the integer comparison.
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28793>
I have no idea why I flipped the order of these to checks vs. the C
code when I wrote the NIR helper. We need to deal with NaN first or
else the fmin will smash NaN to MAX_RGB9E5 and it won't get handled as
NaN.
Fixes: 9981709d8f ("nir/format_convert: Add a function to pack RGB9_E5 formats")
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28793>
Since nir_opt_varyings requires scalar IO and thus all drivers have to
scalarize it, this gives the option to re-vectorize IO after that.
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29406>
This came up while reviewing
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29398 ... Possibly
this intrinsic should be renamed to load_smem_constant_amd for consistency with
load_global_constant. But if we're not going to convey constantness in the
intrinsic name, let's at least document the restriction, because NIR's optimizer
relies on it.
(I didn't inspect every call site, but it looks like load_smem_amd is just used
for descriptor loads so there's no bug to fix.)
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Rhys Perry <pendingchaos02@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29743>
Most passes want to preserve this specific combination of metadata, so let's add
an alias for the combination. The alias communicates that the control flow graph
is preserved, rather than a particular statement about e.g. dominance
preservation.
You don't need to understand dominance to write a simple
nir_shader_instructions_pass. And since you were going to cargo cult the
metadata anyway, this way you'll cargo cult a version you're more likely to
understand.
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Acked-by: Karol Herbst <kherbst@redhat.com>
Reviewed-by: Konstantin Seurer <konstantin.seurer@gmail.com>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29745>
This flag is mostly redundant with uses_discard and was only
introduced to implement demote with LLVM when it didn't have
that intrinsic.
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27617>
The semantics of discard differ between GLSL and HLSL and
their various implementations. Subsequently, numerous application
bugs occurred and SPV_EXT_demote_to_helper_invocation was written
in order to clarify the behavior. In NIR, we now have 3 different
intrinsics for 2 things, and while demote and terminate have clear
semantics, discard still doesn't and can mean either of the two.
This patch entirely removes nir_intrinsic_discard and
nir_intrinsic_discard_if and replaces all occurences either with
nir_intrinsic_terminate{_if} or nir_intrinsic_demote{_if} in the
case that the NIR option 'discard_is_demote' is being set.
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Reviewed-by: Faith Ekstrand <faith.ekstrand@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27617>
