Re: [Mesa-dev] [PATCH 05/28] Revert "spirv: Don’t check for NaN for most OpFOrd* comparisons"
On Fri, Dec 7, 2018 at 1:16 AM Ian Romanick wrote:
>
> On 12/05/2018 10:12 AM, Connor Abbott wrote:
> > This won't work, since this optimization in nir_opt_algebraic will undo it:
> >
> > # For any float comparison operation, "cmp", if you have "a == a && a cmp b"
> > # then the "a == a" is redundant because it's equivalent to "a is not NaN"
> > # and, if a is a NaN then the second comparison will fail anyway.
> > for op in ['flt', 'fge', 'feq']:
> >optimizations += [
> > (('iand', ('feq', a, a), (op, a, b)), (op, a, b)),
> > (('iand', ('feq', a, a), (op, b, a)), (op, b, a)),
> >]
> >
> > and then this optimization might change the behavior for NaN's:
> >
> ># Comparison simplifications
> >(('~inot', ('flt', a, b)), ('fge', a, b)),
> >(('~inot', ('fge', a, b)), ('flt', a, b)),
> >(('~inot', ('feq', a, b)), ('fne', a, b)),
> >(('~inot', ('fne', a, b)), ('feq', a, b)),
> >
> > The topic of NaN's and comparisons in NIR has been discussed several
> > times before, most recently with this thread:
> > https://lists.freedesktop.org/archives/mesa-dev/2018-December/210780.html
>
> These two optimizations do not play well together. Each by itself is
> valid, but the combination is not. As I mentioned in the previous
> thread, I believe the first change should mark the resulting (op, a, b)
> as precise. That would prevent the second optimization from triggering.
> I don't think nir_opt_algebraic has way to do this, but it doesn't seem
> like it would be that hard to add. Regardless of what happens with the
> rest, breaking the cumulative effect of these optimizations is necessary.
Well, the entire reason for the first optimization was the code that's
being reinstated in this commit, so we could probably get rid of it
once we get rid of the need for it. If user code actually does the
extra NaN check, then yeah, we might need a way to mark this as
precise. One long-term thing would be splitting up the "precise" flag
into a per-instruction bitflag with all the options introduced in this
series, so we can mark the compare as "preserves NaN" and still let
other NaN-preserving but e.g. incorrect wrt signed zero optimizations
take place.
>
> > Given that this language got added to the Vulkan spec: "By default,
> > the implementation may perform optimizations on half, single, or
> > double-precision floating-point instructions respectively that ignore
> > sign of a zero, or assume that arguments and results are not Nans or
> > ±∞" I think we should probably do the following:
> >
> > - Fix the CTS tests that prompted this whole block of code to only
> > check the result of comparing NaN when this extension is available and
> > NaN's are preserved.
> > - nir_op_fne must be unordered already, regardless of what
> > floating-point options the user asked for, since it's used to
> > implement isNan() already. We should probably also define nir_op_feq
> > to be ordered. So we don't have to do anything with them, and !(a ==
> > b) == (a == b) is guaranteed.
> > - Define fgt and fle to be ordered, as this is what every backend
> > already does. No need to add unnecessary NaN checks.
> > - Disable the comparison simplifications (except for the fne one,
> > which shouldn't be marked imprecise as it is now) when preserve-nan is
> > set. I think there are a few other imprecise transforms that also need
> > to be disabled.
>
> Even with the work that I've been doing this week, removing these
> optimizations still hurts (quite dramatically in a few cases) over 1500
> shaders in shader-db.
That's why I said "when preserve-nan is set." When an application sets
this flag, it means they really do want NaN to work correctly,
whatever the cost, so the combination of the first and second
optimizations is invalid.
> By and large, applications go to great lengths to
> avoid things that could generate NaN. If a calculation generates NaN in
> a graphics shader, you've already lost. As a result, these hurt shaders
> work as-is. Adding an extra 8% instructions to a working shader doesn't
> help anyone.
>
> Based on the commit message in d55835b8bdf0, my hypothesis is that
> disabling the combination of the two sets of optimizations won't
> negatively affect any shaders.
It will, since it makes every comparison precise for SPIR-V. This
won't hurt only if you never emit the NaN checks unless preserve-nan
is set. But this will just emit some pointless NaN checks in vtn when
preserve-nan is enabled and then throw them away immediately in
opt_algebraic while marking the comparison as precise. So it helps for
the theoretical case where the user themselves adds the NaN check, but
it's orthogonal to getting it right for SPIR-V. Disabling the second
transform when preserve-nan is set is better than marking comparisons
as precise when preserve-nan is set since it lets us seperate
optimizations which aren't NaN-safe from those that aren't
signed-zero-safe, etc.
>
> > - (optional) Add fgtu and fleu opcodes for
Re: [Mesa-dev] [PATCH 05/28] Revert "spirv: Don’t check for NaN for most OpFOrd* comparisons"
On 12/05/2018 10:12 AM, Connor Abbott wrote:
> This won't work, since this optimization in nir_opt_algebraic will undo it:
>
> # For any float comparison operation, "cmp", if you have "a == a && a cmp b"
> # then the "a == a" is redundant because it's equivalent to "a is not NaN"
> # and, if a is a NaN then the second comparison will fail anyway.
> for op in ['flt', 'fge', 'feq']:
>optimizations += [
> (('iand', ('feq', a, a), (op, a, b)), (op, a, b)),
> (('iand', ('feq', a, a), (op, b, a)), (op, b, a)),
>]
>
> and then this optimization might change the behavior for NaN's:
>
># Comparison simplifications
>(('~inot', ('flt', a, b)), ('fge', a, b)),
>(('~inot', ('fge', a, b)), ('flt', a, b)),
>(('~inot', ('feq', a, b)), ('fne', a, b)),
>(('~inot', ('fne', a, b)), ('feq', a, b)),
>
> The topic of NaN's and comparisons in NIR has been discussed several
> times before, most recently with this thread:
> https://lists.freedesktop.org/archives/mesa-dev/2018-December/210780.html
These two optimizations do not play well together. Each by itself is
valid, but the combination is not. As I mentioned in the previous
thread, I believe the first change should mark the resulting (op, a, b)
as precise. That would prevent the second optimization from triggering.
I don't think nir_opt_algebraic has way to do this, but it doesn't seem
like it would be that hard to add. Regardless of what happens with the
rest, breaking the cumulative effect of these optimizations is necessary.
> Given that this language got added to the Vulkan spec: "By default,
> the implementation may perform optimizations on half, single, or
> double-precision floating-point instructions respectively that ignore
> sign of a zero, or assume that arguments and results are not Nans or
> ±∞" I think we should probably do the following:
>
> - Fix the CTS tests that prompted this whole block of code to only
> check the result of comparing NaN when this extension is available and
> NaN's are preserved.
> - nir_op_fne must be unordered already, regardless of what
> floating-point options the user asked for, since it's used to
> implement isNan() already. We should probably also define nir_op_feq
> to be ordered. So we don't have to do anything with them, and !(a ==
> b) == (a == b) is guaranteed.
> - Define fgt and fle to be ordered, as this is what every backend
> already does. No need to add unnecessary NaN checks.
> - Disable the comparison simplifications (except for the fne one,
> which shouldn't be marked imprecise as it is now) when preserve-nan is
> set. I think there are a few other imprecise transforms that also need
> to be disabled.
Even with the work that I've been doing this week, removing these
optimizations still hurts (quite dramatically in a few cases) over 1500
shaders in shader-db. By and large, applications go to great lengths to
avoid things that could generate NaN. If a calculation generates NaN in
a graphics shader, you've already lost. As a result, these hurt shaders
work as-is. Adding an extra 8% instructions to a working shader doesn't
help anyone.
Based on the commit message in d55835b8bdf0, my hypothesis is that
disabling the combination of the two sets of optimizations won't
negatively affect any shaders.
> - (optional) Add fgtu and fleu opcodes for unordered comparisons. This
> might help backends which can do these in only one instruction. Even
> if we don't do this, these can be implemented as not (fle a, b) and
> not (fgt a, b) respectively, which is fewer instructions than the
> current lowering.
> - (optional) Add fequ and fneo opcodes that do unordered equal and
> ordered not-equal, respectively. Otherwise they have to be implemented
> with explicit NaN checks like now.
>
> On Wed, Dec 5, 2018 at 4:56 PM Samuel Iglesias Gonsálvez
> wrote:
>>
>> This reverts commit c4ab1bdcc9710e3c7cc7115d3be9c69b7e7712ef. We need
>> to check the arguments looking for NaNs, because they can introduce
>> failures in tests for FOrd*, specially when running
>> VK_KHR_shader_float_control tests in CTS.
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez
>> ---
>> src/compiler/spirv/vtn_alu.c | 17 +++--
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
>> index dc6fedc9129..629b57560ca 100644
>> --- a/src/compiler/spirv/vtn_alu.c
>> +++ b/src/compiler/spirv/vtn_alu.c
>> @@ -535,18 +535,23 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp opcode,
>>break;
>> }
>>
>> - case SpvOpFOrdNotEqual: {
>> - /* For all the SpvOpFOrd* comparisons apart from NotEqual, the value
>> - * from the ALU will probably already be false if the operands are not
>> - * ordered so we don’t need to handle it specially.
>> - */
>> + case SpvOpFOrdEqual:
>> + case SpvOpFOrdNotEqual:
>> + case SpvOpFOrdLessThan:
>> + case SpvOpFOrdGreaterThan:
>> + case SpvOpFOrdLessThanEqual:
>> +
Re: [Mesa-dev] [PATCH 05/28] Revert "spirv: Don’t check for NaN for most OpFOrd* comparisons"
I sent a series last week which does almost everything Connor mentioned...
On December 5, 2018 12:12:50 Connor Abbott wrote:
This won't work, since this optimization in nir_opt_algebraic will undo it:
# For any float comparison operation, "cmp", if you have "a == a && a cmp b"
# then the "a == a" is redundant because it's equivalent to "a is not NaN"
# and, if a is a NaN then the second comparison will fail anyway.
for op in ['flt', 'fge', 'feq']:
optimizations += [
(('iand', ('feq', a, a), (op, a, b)), (op, a, b)),
(('iand', ('feq', a, a), (op, b, a)), (op, b, a)),
]
and then this optimization might change the behavior for NaN's:
# Comparison simplifications
(('~inot', ('flt', a, b)), ('fge', a, b)),
(('~inot', ('fge', a, b)), ('flt', a, b)),
(('~inot', ('feq', a, b)), ('fne', a, b)),
(('~inot', ('fne', a, b)), ('feq', a, b)),
The topic of NaN's and comparisons in NIR has been discussed several
times before, most recently with this thread:
https://lists.freedesktop.org/archives/mesa-dev/2018-December/210780.html
Given that this language got added to the Vulkan spec: "By default,
the implementation may perform optimizations on half, single, or
double-precision floating-point instructions respectively that ignore
sign of a zero, or assume that arguments and results are not Nans or
±∞" I think we should probably do the following:
- Fix the CTS tests that prompted this whole block of code to only
check the result of comparing NaN when this extension is available and
NaN's are preserved.
- nir_op_fne must be unordered already, regardless of what
floating-point options the user asked for, since it's used to
implement isNan() already. We should probably also define nir_op_feq
to be ordered. So we don't have to do anything with them, and !(a ==
b) == (a == b) is guaranteed.
- Define fgt and fle to be ordered, as this is what every backend
already does. No need to add unnecessary NaN checks.
- Disable the comparison simplifications (except for the fne one,
which shouldn't be marked imprecise as it is now) when preserve-nan is
set. I think there are a few other imprecise transforms that also need
to be disabled.
- (optional) Add fgtu and fleu opcodes for unordered comparisons. This
might help backends which can do these in only one instruction. Even
if we don't do this, these can be implemented as not (fle a, b) and
not (fgt a, b) respectively, which is fewer instructions than the
current lowering.
- (optional) Add fequ and fneo opcodes that do unordered equal and
ordered not-equal, respectively. Otherwise they have to be implemented
with explicit NaN checks like now.
On Wed, Dec 5, 2018 at 4:56 PM Samuel Iglesias Gonsálvez
wrote:
This reverts commit c4ab1bdcc9710e3c7cc7115d3be9c69b7e7712ef. We need
to check the arguments looking for NaNs, because they can introduce
failures in tests for FOrd*, specially when running
VK_KHR_shader_float_control tests in CTS.
Signed-off-by: Samuel Iglesias Gonsálvez
---
src/compiler/spirv/vtn_alu.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
index dc6fedc9129..629b57560ca 100644
--- a/src/compiler/spirv/vtn_alu.c
+++ b/src/compiler/spirv/vtn_alu.c
@@ -535,18 +535,23 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp opcode,
break;
}
- case SpvOpFOrdNotEqual: {
- /* For all the SpvOpFOrd* comparisons apart from NotEqual, the value
- * from the ALU will probably already be false if the operands are not
- * ordered so we don’t need to handle it specially.
- */
+ case SpvOpFOrdEqual:
+ case SpvOpFOrdNotEqual:
+ case SpvOpFOrdLessThan:
+ case SpvOpFOrdGreaterThan:
+ case SpvOpFOrdLessThanEqual:
+ case SpvOpFOrdGreaterThanEqual: {
bool swap;
unsigned src_bit_size = glsl_get_bit_size(vtn_src[0]->type);
unsigned dst_bit_size = glsl_get_bit_size(type);
nir_op op = vtn_nir_alu_op_for_spirv_opcode(b, opcode, ,
src_bit_size, dst_bit_size);
- assert(!swap);
+ if (swap) {
+ nir_ssa_def *tmp = src[0];
+ src[0] = src[1];
+ src[1] = tmp;
+ }
val->ssa->def =
nir_iand(>nb,
--
2.19.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/28] Revert "spirv: Don’t check for NaN for most OpFOrd* comparisons"
This won't work, since this optimization in nir_opt_algebraic will undo it:
# For any float comparison operation, "cmp", if you have "a == a && a cmp b"
# then the "a == a" is redundant because it's equivalent to "a is not NaN"
# and, if a is a NaN then the second comparison will fail anyway.
for op in ['flt', 'fge', 'feq']:
optimizations += [
(('iand', ('feq', a, a), (op, a, b)), (op, a, b)),
(('iand', ('feq', a, a), (op, b, a)), (op, b, a)),
]
and then this optimization might change the behavior for NaN's:
# Comparison simplifications
(('~inot', ('flt', a, b)), ('fge', a, b)),
(('~inot', ('fge', a, b)), ('flt', a, b)),
(('~inot', ('feq', a, b)), ('fne', a, b)),
(('~inot', ('fne', a, b)), ('feq', a, b)),
The topic of NaN's and comparisons in NIR has been discussed several
times before, most recently with this thread:
https://lists.freedesktop.org/archives/mesa-dev/2018-December/210780.html
Given that this language got added to the Vulkan spec: "By default,
the implementation may perform optimizations on half, single, or
double-precision floating-point instructions respectively that ignore
sign of a zero, or assume that arguments and results are not Nans or
±∞" I think we should probably do the following:
- Fix the CTS tests that prompted this whole block of code to only
check the result of comparing NaN when this extension is available and
NaN's are preserved.
- nir_op_fne must be unordered already, regardless of what
floating-point options the user asked for, since it's used to
implement isNan() already. We should probably also define nir_op_feq
to be ordered. So we don't have to do anything with them, and !(a ==
b) == (a == b) is guaranteed.
- Define fgt and fle to be ordered, as this is what every backend
already does. No need to add unnecessary NaN checks.
- Disable the comparison simplifications (except for the fne one,
which shouldn't be marked imprecise as it is now) when preserve-nan is
set. I think there are a few other imprecise transforms that also need
to be disabled.
- (optional) Add fgtu and fleu opcodes for unordered comparisons. This
might help backends which can do these in only one instruction. Even
if we don't do this, these can be implemented as not (fle a, b) and
not (fgt a, b) respectively, which is fewer instructions than the
current lowering.
- (optional) Add fequ and fneo opcodes that do unordered equal and
ordered not-equal, respectively. Otherwise they have to be implemented
with explicit NaN checks like now.
On Wed, Dec 5, 2018 at 4:56 PM Samuel Iglesias Gonsálvez
wrote:
>
> This reverts commit c4ab1bdcc9710e3c7cc7115d3be9c69b7e7712ef. We need
> to check the arguments looking for NaNs, because they can introduce
> failures in tests for FOrd*, specially when running
> VK_KHR_shader_float_control tests in CTS.
>
> Signed-off-by: Samuel Iglesias Gonsálvez
> ---
> src/compiler/spirv/vtn_alu.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
> index dc6fedc9129..629b57560ca 100644
> --- a/src/compiler/spirv/vtn_alu.c
> +++ b/src/compiler/spirv/vtn_alu.c
> @@ -535,18 +535,23 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp opcode,
>break;
> }
>
> - case SpvOpFOrdNotEqual: {
> - /* For all the SpvOpFOrd* comparisons apart from NotEqual, the value
> - * from the ALU will probably already be false if the operands are not
> - * ordered so we don’t need to handle it specially.
> - */
> + case SpvOpFOrdEqual:
> + case SpvOpFOrdNotEqual:
> + case SpvOpFOrdLessThan:
> + case SpvOpFOrdGreaterThan:
> + case SpvOpFOrdLessThanEqual:
> + case SpvOpFOrdGreaterThanEqual: {
>bool swap;
>unsigned src_bit_size = glsl_get_bit_size(vtn_src[0]->type);
>unsigned dst_bit_size = glsl_get_bit_size(type);
>nir_op op = vtn_nir_alu_op_for_spirv_opcode(b, opcode, ,
>src_bit_size,
> dst_bit_size);
>
> - assert(!swap);
> + if (swap) {
> + nir_ssa_def *tmp = src[0];
> + src[0] = src[1];
> + src[1] = tmp;
> + }
>
>val->ssa->def =
> nir_iand(>nb,
> --
> 2.19.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
