Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On Fri, May 8, 2015 at 3:32 PM, Ian Romanick wrote:
> On 05/08/2015 03:31 PM, Ian Romanick wrote:
>> On 05/08/2015 03:25 PM, Jason Ekstrand wrote:
>>> On Fri, May 8, 2015 at 3:20 PM, Ian Romanick wrote:
On 05/08/2015 11:55 AM, Jason Ekstrand wrote:
> On Fri, May 8, 2015 at 11:53 AM, Jason Ekstrand
> wrote:
>> total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
>> instructions in affected programs: 1330548 -> 1315224 (-1.15%)
>> helped:5797
>> HURT: 76
>
> I'm doing some looking into the hurt programs. It seems as if there
> are some very strange interatctions between flrp and ffma. I'm still
> working out exactly how to fix it up.
Yes, I noticed this too. Did you see my reply to Ken earlier today?
The problem I noticed /seems/ restricted to cases where the would-be
interpolant is scalar but the other values are vector.
>>>
>>> It would surprise me a lot of that mattered. At the point where we do
>>> opt_algebraic in NIR, we've already scalarized things. That said,
>>> there is a *lot* going on in an optimization loop so anything's
>>> possible.
>>
>> If I take the shader_runner test that I included in the e-mail to Ken
>> and s/float alpha/vec3 alpha/ I get LRPs. I made some obvious tweaks to
>> opt_algebraic to handle the mix of scalar and vector, and something like
>> 150 shaders were helped.
Ok, that's weird. I really have no idea why that would make a
difference. Unless, of course, it affects the optimizations that GLSL
IR is able to do which then, in turn, affects NIR. That would make
some sense.
>> I spent about an hour digging into it, and I came up dry. I have tried
>> adding some rules to nir_opt_algebraic.py to convert the fmul/ffma to
>> flrp, and I couldn't get a break point at the nir_opt_ffma to trigger.
>> I was going to ask about it at the office on Monday, but it came up on
>> the list first.
>
> FWIW, those rules were:
>
>(('ffma', b, c, ('fmul', a, ('fadd', 1.0, ('fneg', c, ('flrp', a, b,
> c), '!options->lower_flrp'),
>(('ffma', c, b, ('fmul', a, ('fadd', 1.0, ('fneg', c, ('flrp', a, b,
> c), '!options->lower_flrp'),
Did you add them to optimizations or late_optimizations? The late
ones get run after opt_ffma and the others don't.
I poked around for an hour or two today with similar optimizations (I
actually had 6) but I wasn't able to get it to do any better.
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On 05/08/2015 03:31 PM, Ian Romanick wrote:
> On 05/08/2015 03:25 PM, Jason Ekstrand wrote:
>> On Fri, May 8, 2015 at 3:20 PM, Ian Romanick wrote:
>>> On 05/08/2015 11:55 AM, Jason Ekstrand wrote:
On Fri, May 8, 2015 at 11:53 AM, Jason Ekstrand
wrote:
> total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
> instructions in affected programs: 1330548 -> 1315224 (-1.15%)
> helped:5797
> HURT: 76
I'm doing some looking into the hurt programs. It seems as if there
are some very strange interatctions between flrp and ffma. I'm still
working out exactly how to fix it up.
>>>
>>> Yes, I noticed this too. Did you see my reply to Ken earlier today?
>>> The problem I noticed /seems/ restricted to cases where the would-be
>>> interpolant is scalar but the other values are vector.
>>
>> It would surprise me a lot of that mattered. At the point where we do
>> opt_algebraic in NIR, we've already scalarized things. That said,
>> there is a *lot* going on in an optimization loop so anything's
>> possible.
>
> If I take the shader_runner test that I included in the e-mail to Ken
> and s/float alpha/vec3 alpha/ I get LRPs. I made some obvious tweaks to
> opt_algebraic to handle the mix of scalar and vector, and something like
> 150 shaders were helped.
>
> I spent about an hour digging into it, and I came up dry. I have tried
> adding some rules to nir_opt_algebraic.py to convert the fmul/ffma to
> flrp, and I couldn't get a break point at the nir_opt_ffma to trigger.
> I was going to ask about it at the office on Monday, but it came up on
> the list first.
FWIW, those rules were:
(('ffma', b, c, ('fmul', a, ('fadd', 1.0, ('fneg', c, ('flrp', a, b, c),
'!options->lower_flrp'),
(('ffma', c, b, ('fmul', a, ('fadd', 1.0, ('fneg', c, ('flrp', a, b, c),
'!options->lower_flrp'),
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On 05/08/2015 03:25 PM, Jason Ekstrand wrote: > On Fri, May 8, 2015 at 3:20 PM, Ian Romanick wrote: >> On 05/08/2015 11:55 AM, Jason Ekstrand wrote: >>> On Fri, May 8, 2015 at 11:53 AM, Jason Ekstrand >>> wrote: total instructions in shared programs: 7152330 -> 7137006 (-0.21%) instructions in affected programs: 1330548 -> 1315224 (-1.15%) helped:5797 HURT: 76 >>> >>> I'm doing some looking into the hurt programs. It seems as if there >>> are some very strange interatctions between flrp and ffma. I'm still >>> working out exactly how to fix it up. >> >> Yes, I noticed this too. Did you see my reply to Ken earlier today? >> The problem I noticed /seems/ restricted to cases where the would-be >> interpolant is scalar but the other values are vector. > > It would surprise me a lot of that mattered. At the point where we do > opt_algebraic in NIR, we've already scalarized things. That said, > there is a *lot* going on in an optimization loop so anything's > possible. If I take the shader_runner test that I included in the e-mail to Ken and s/float alpha/vec3 alpha/ I get LRPs. I made some obvious tweaks to opt_algebraic to handle the mix of scalar and vector, and something like 150 shaders were helped. I spent about an hour digging into it, and I came up dry. I have tried adding some rules to nir_opt_algebraic.py to convert the fmul/ffma to flrp, and I couldn't get a break point at the nir_opt_ffma to trigger. I was going to ask about it at the office on Monday, but it came up on the list first. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On Fri, May 8, 2015 at 3:20 PM, Ian Romanick wrote:
> On 05/08/2015 11:55 AM, Jason Ekstrand wrote:
>> On Fri, May 8, 2015 at 11:53 AM, Jason Ekstrand wrote:
>>> total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
>>> instructions in affected programs: 1330548 -> 1315224 (-1.15%)
>>> helped:5797
>>> HURT: 76
>>
>> I'm doing some looking into the hurt programs. It seems as if there
>> are some very strange interatctions between flrp and ffma. I'm still
>> working out exactly how to fix it up.
>
> Yes, I noticed this too. Did you see my reply to Ken earlier today?
> The problem I noticed /seems/ restricted to cases where the would-be
> interpolant is scalar but the other values are vector.
It would surprise me a lot of that mattered. At the point where we do
opt_algebraic in NIR, we've already scalarized things. That said,
there is a *lot* going on in an optimization loop so anything's
possible.
--Jason
>> --Jason
>>
>>> GAINED:0
>>> LOST: 8
>>> ---
>>> src/glsl/nir/nir_search.c | 11 +++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
>>> index d86655b..4c83349 100644
>>> --- a/src/glsl/nir/nir_search.c
>>> +++ b/src/glsl/nir/nir_search.c
>>> @@ -199,6 +199,10 @@ match_expression(const nir_search_expression *expr,
>>> nir_alu_instr *instr,
>>>}
>>> }
>>>
>>> + /* Stash off the current variables_seen bitmask. This way we can
>>> +* restore it prior to matching in the commutative case below. */
>>> + unsigned variables_seen_stash = state->variables_seen;
>>> +
>>> bool matched = true;
>>> for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
>>>/* If the source is an explicitly sized source, then we need to reset
>>> @@ -221,6 +225,13 @@ match_expression(const nir_search_expression *expr,
>>> nir_alu_instr *instr,
>>>
>>> if (nir_op_infos[instr->op].algebraic_properties &
>>> NIR_OP_IS_COMMUTATIVE) {
>>>assert(nir_op_infos[instr->op].num_inputs == 2);
>>> +
>>> + /* Restore the variables_seen bitmask. If we don't do this, then we
>>> + * could end up with an erroneous failure due to variables found in
>>> the
>>> + * first match attempt above not matching those in the second.
>>> + */
>>> + state->variables_seen = variables_seen_stash;
>>> +
>>>if (!match_value(expr->srcs[0], instr, 1, num_components,
>>> swizzle, state))
>>> return false;
>>> --
>>> 2.4.0
>>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On 05/08/2015 11:55 AM, Jason Ekstrand wrote:
> On Fri, May 8, 2015 at 11:53 AM, Jason Ekstrand wrote:
>> total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
>> instructions in affected programs: 1330548 -> 1315224 (-1.15%)
>> helped:5797
>> HURT: 76
>
> I'm doing some looking into the hurt programs. It seems as if there
> are some very strange interatctions between flrp and ffma. I'm still
> working out exactly how to fix it up.
Yes, I noticed this too. Did you see my reply to Ken earlier today?
The problem I noticed /seems/ restricted to cases where the would-be
interpolant is scalar but the other values are vector.
> --Jason
>
>> GAINED:0
>> LOST: 8
>> ---
>> src/glsl/nir/nir_search.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
>> index d86655b..4c83349 100644
>> --- a/src/glsl/nir/nir_search.c
>> +++ b/src/glsl/nir/nir_search.c
>> @@ -199,6 +199,10 @@ match_expression(const nir_search_expression *expr,
>> nir_alu_instr *instr,
>>}
>> }
>>
>> + /* Stash off the current variables_seen bitmask. This way we can
>> +* restore it prior to matching in the commutative case below. */
>> + unsigned variables_seen_stash = state->variables_seen;
>> +
>> bool matched = true;
>> for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
>>/* If the source is an explicitly sized source, then we need to reset
>> @@ -221,6 +225,13 @@ match_expression(const nir_search_expression *expr,
>> nir_alu_instr *instr,
>>
>> if (nir_op_infos[instr->op].algebraic_properties &
>> NIR_OP_IS_COMMUTATIVE) {
>>assert(nir_op_infos[instr->op].num_inputs == 2);
>> +
>> + /* Restore the variables_seen bitmask. If we don't do this, then we
>> + * could end up with an erroneous failure due to variables found in
>> the
>> + * first match attempt above not matching those in the second.
>> + */
>> + state->variables_seen = variables_seen_stash;
>> +
>>if (!match_value(expr->srcs[0], instr, 1, num_components,
>> swizzle, state))
>> return false;
>> --
>> 2.4.0
>>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On Fri, May 8, 2015 at 2:53 PM, Jason Ekstrand wrote:
> total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
> instructions in affected programs: 1330548 -> 1315224 (-1.15%)
> helped:5797
> HURT: 76
> GAINED:0
> LOST: 8
> ---
> src/glsl/nir/nir_search.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
> index d86655b..4c83349 100644
> --- a/src/glsl/nir/nir_search.c
> +++ b/src/glsl/nir/nir_search.c
> @@ -199,6 +199,10 @@ match_expression(const nir_search_expression *expr,
> nir_alu_instr *instr,
>}
> }
>
> + /* Stash off the current variables_seen bitmask. This way we can
> +* restore it prior to matching in the commutative case below. */
Puth the */ on the next line, and this is
Reviewed-by: Connor Abbott
> + unsigned variables_seen_stash = state->variables_seen;
> +
> bool matched = true;
> for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
>/* If the source is an explicitly sized source, then we need to reset
> @@ -221,6 +225,13 @@ match_expression(const nir_search_expression *expr,
> nir_alu_instr *instr,
>
> if (nir_op_infos[instr->op].algebraic_properties & NIR_OP_IS_COMMUTATIVE)
> {
>assert(nir_op_infos[instr->op].num_inputs == 2);
> +
> + /* Restore the variables_seen bitmask. If we don't do this, then we
> + * could end up with an erroneous failure due to variables found in the
> + * first match attempt above not matching those in the second.
> + */
> + state->variables_seen = variables_seen_stash;
> +
>if (!match_value(expr->srcs[0], instr, 1, num_components,
> swizzle, state))
> return false;
> --
> 2.4.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
On Fri, May 8, 2015 at 11:53 AM, Jason Ekstrand wrote:
> total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
> instructions in affected programs: 1330548 -> 1315224 (-1.15%)
> helped:5797
> HURT: 76
I'm doing some looking into the hurt programs. It seems as if there
are some very strange interatctions between flrp and ffma. I'm still
working out exactly how to fix it up.
--Jason
> GAINED:0
> LOST: 8
> ---
> src/glsl/nir/nir_search.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
> index d86655b..4c83349 100644
> --- a/src/glsl/nir/nir_search.c
> +++ b/src/glsl/nir/nir_search.c
> @@ -199,6 +199,10 @@ match_expression(const nir_search_expression *expr,
> nir_alu_instr *instr,
>}
> }
>
> + /* Stash off the current variables_seen bitmask. This way we can
> +* restore it prior to matching in the commutative case below. */
> + unsigned variables_seen_stash = state->variables_seen;
> +
> bool matched = true;
> for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
>/* If the source is an explicitly sized source, then we need to reset
> @@ -221,6 +225,13 @@ match_expression(const nir_search_expression *expr,
> nir_alu_instr *instr,
>
> if (nir_op_infos[instr->op].algebraic_properties & NIR_OP_IS_COMMUTATIVE)
> {
>assert(nir_op_infos[instr->op].num_inputs == 2);
> +
> + /* Restore the variables_seen bitmask. If we don't do this, then we
> + * could end up with an erroneous failure due to variables found in the
> + * first match attempt above not matching those in the second.
> + */
> + state->variables_seen = variables_seen_stash;
> +
>if (!match_value(expr->srcs[0], instr, 1, num_components,
> swizzle, state))
> return false;
> --
> 2.4.0
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] nir/search: Save/restore the variables_seen bitmask when matching
total instructions in shared programs: 7152330 -> 7137006 (-0.21%)
instructions in affected programs: 1330548 -> 1315224 (-1.15%)
helped:5797
HURT: 76
GAINED:0
LOST: 8
---
src/glsl/nir/nir_search.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/glsl/nir/nir_search.c b/src/glsl/nir/nir_search.c
index d86655b..4c83349 100644
--- a/src/glsl/nir/nir_search.c
+++ b/src/glsl/nir/nir_search.c
@@ -199,6 +199,10 @@ match_expression(const nir_search_expression *expr,
nir_alu_instr *instr,
}
}
+ /* Stash off the current variables_seen bitmask. This way we can
+* restore it prior to matching in the commutative case below. */
+ unsigned variables_seen_stash = state->variables_seen;
+
bool matched = true;
for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
/* If the source is an explicitly sized source, then we need to reset
@@ -221,6 +225,13 @@ match_expression(const nir_search_expression *expr,
nir_alu_instr *instr,
if (nir_op_infos[instr->op].algebraic_properties & NIR_OP_IS_COMMUTATIVE) {
assert(nir_op_infos[instr->op].num_inputs == 2);
+
+ /* Restore the variables_seen bitmask. If we don't do this, then we
+ * could end up with an erroneous failure due to variables found in the
+ * first match attempt above not matching those in the second.
+ */
+ state->variables_seen = variables_seen_stash;
+
if (!match_value(expr->srcs[0], instr, 1, num_components,
swizzle, state))
return false;
--
2.4.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
