Re: [Mesa-dev] [PATCH] nir: Copy-propagate vecN operations that are actually moves
On Feb 20, 2015 9:27 AM, "Matt Turner" wrote: > > On Fri, Feb 20, 2015 at 9:23 AM, Jason Ekstrand wrote: > > > > > > On Thu, Feb 19, 2015 at 11:03 PM, Matt Turner wrote: > >> > >> On Thu, Feb 19, 2015 at 11:01 PM, Connor Abbott > >> wrote: > >> > I agree with Ken that the regressions are small enough, and it seems > >> > they're mostly stuff we can prevent by being smarter when doing the > >> > sel peephole, so it seems like the cleanup that will probably help > >> > other passes is worth it. > >> > >> So, usually we do that as a preparatory patch. Why aren't we doing that > >> here? > > > > > > Do what in a preparatory patch? Fix up the sel peephole to be able to > > handle "if (foo) bar = baz;"? Sure, I can put that patch together. > > I thought that this would get turned into a conditional select as a > side-effect of going out of SSA anyway (assuming baz was > unconditionally set before the if)? Are these shaders not setting baz > unconditionally? The NIR peephole only triggers if the only instructions in the if or else are moves. In this particular case, it was a uniform load. Before the change, it was lowered to moves in both sides of the if and, since it ended up being a push constant, got peepholed. After the change, it was lowered to moves on only one side of the if (which is theoretically better) and the select didn't trigger. > >> > >> NIR instruction counts is not the metric we care about. > > > > > > No, but cleaning things up means that we can do other optimizations better. > > Also, in each of those cases, the non-ssa NIR code was better it was just > > less cleanable by the backend. We need to work on that, but I don't think > > it's an indicator of a problem. > > --Jason > > Okay, that seems fine. I wouldn't bother updating the SEL peephole. It > sounds like a number of the regressions we have are related to it not > handling something so we might need to do a larger evaluation. Yeah, it just needs to handle one-sided it's better. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir: Copy-propagate vecN operations that are actually moves
On Fri, Feb 20, 2015 at 9:23 AM, Jason Ekstrand wrote: > > > On Thu, Feb 19, 2015 at 11:03 PM, Matt Turner wrote: >> >> On Thu, Feb 19, 2015 at 11:01 PM, Connor Abbott >> wrote: >> > I agree with Ken that the regressions are small enough, and it seems >> > they're mostly stuff we can prevent by being smarter when doing the >> > sel peephole, so it seems like the cleanup that will probably help >> > other passes is worth it. >> >> So, usually we do that as a preparatory patch. Why aren't we doing that >> here? > > > Do what in a preparatory patch? Fix up the sel peephole to be able to > handle "if (foo) bar = baz;"? Sure, I can put that patch together. I thought that this would get turned into a conditional select as a side-effect of going out of SSA anyway (assuming baz was unconditionally set before the if)? Are these shaders not setting baz unconditionally? >> >> NIR instruction counts is not the metric we care about. > > > No, but cleaning things up means that we can do other optimizations better. > Also, in each of those cases, the non-ssa NIR code was better it was just > less cleanable by the backend. We need to work on that, but I don't think > it's an indicator of a problem. > --Jason Okay, that seems fine. I wouldn't bother updating the SEL peephole. It sounds like a number of the regressions we have are related to it not handling something so we might need to do a larger evaluation. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir: Copy-propagate vecN operations that are actually moves
On Thu, Feb 19, 2015 at 11:03 PM, Matt Turner wrote: > On Thu, Feb 19, 2015 at 11:01 PM, Connor Abbott > wrote: > > I agree with Ken that the regressions are small enough, and it seems > > they're mostly stuff we can prevent by being smarter when doing the > > sel peephole, so it seems like the cleanup that will probably help > > other passes is worth it. > > So, usually we do that as a preparatory patch. Why aren't we doing that > here? > Do what in a preparatory patch? Fix up the sel peephole to be able to handle "if (foo) bar = baz;"? Sure, I can put that patch together. > NIR instruction counts is not the metric we care about. > No, but cleaning things up means that we can do other optimizations better. Also, in each of those cases, the non-ssa NIR code was better it was just less cleanable by the backend. We need to work on that, but I don't think it's an indicator of a problem. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir: Copy-propagate vecN operations that are actually moves
On Thu, Feb 19, 2015 at 11:01 PM, Connor Abbott wrote: > I agree with Ken that the regressions are small enough, and it seems > they're mostly stuff we can prevent by being smarter when doing the > sel peephole, so it seems like the cleanup that will probably help > other passes is worth it. So, usually we do that as a preparatory patch. Why aren't we doing that here? NIR instruction counts is not the metric we care about. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir: Copy-propagate vecN operations that are actually moves
I agree with Ken that the regressions are small enough, and it seems
they're mostly stuff we can prevent by being smarter when doing the
sel peephole, so it seems like the cleanup that will probably help
other passes is worth it.
Reviewed-by: Connor Abbott
On Fri, Feb 20, 2015 at 1:03 AM, Jason Ekstrand wrote:
> We were already do this for ALU operations but we haven't for non-ALU
> operations. This changes that.
>
> total NIR instructions in shared programs: 2039883 -> 2022338 (-0.86%)
> NIR instructions in affected programs: 1768850 -> 1751305 (-0.99%)
> helped:14244
> HURT: 124
>
> total FS instructions in shared programs: 4083960 -> 4084036 (0.00%)
> FS instructions in affected programs: 7302 -> 7378 (1.04%)
> helped: 12
> HURT: 51
>
> Signed-off-by: Jason Ekstrand
> ---
> src/glsl/nir/nir_opt_copy_propagate.c | 45
> ++-
> 1 file changed, 29 insertions(+), 16 deletions(-)
>
> diff --git a/src/glsl/nir/nir_opt_copy_propagate.c
> b/src/glsl/nir/nir_opt_copy_propagate.c
> index dd0ec01..ee78e5a 100644
> --- a/src/glsl/nir/nir_opt_copy_propagate.c
> +++ b/src/glsl/nir/nir_opt_copy_propagate.c
> @@ -53,22 +53,6 @@ static bool is_move(nir_alu_instr *instr)
>
> }
>
> -static bool
> -is_swizzleless_move(nir_alu_instr *instr)
> -{
> - if (!is_move(instr))
> - return false;
> -
> - for (unsigned i = 0; i < 4; i++) {
> - if (!((instr->dest.write_mask >> i) & 1))
> - break;
> - if (instr->src[0].swizzle[i] != i)
> - return false;
> - }
> -
> - return true;
> -}
> -
> static bool is_vec(nir_alu_instr *instr)
> {
> for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++)
> @@ -80,6 +64,35 @@ static bool is_vec(nir_alu_instr *instr)
>instr->op == nir_op_vec4;
> }
>
> +static bool
> +is_swizzleless_move(nir_alu_instr *instr)
> +{
> + if (is_move(instr)) {
> + for (unsigned i = 0; i < 4; i++) {
> + if (!((instr->dest.write_mask >> i) & 1))
> +break;
> + if (instr->src[0].swizzle[i] != i)
> +return false;
> + }
> + return true;
> + } else if (is_vec(instr)) {
> + nir_ssa_def *def = NULL;
> + for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
> + if (instr->src[i].swizzle[0] != i)
> +return false;
> +
> + if (def == NULL) {
> +def = instr->src[i].src.ssa;
> + } else if (instr->src[i].src.ssa != def) {
> +return false;
> + }
> + }
> + return true;
> + } else {
> + return false;
> + }
> +}
> +
> typedef struct {
> nir_ssa_def *def;
> bool found;
> --
> 2.3.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: Copy-propagate vecN operations that are actually moves
On Thursday, February 19, 2015 10:03:15 PM Jason Ekstrand wrote:
> We were already do this for ALU operations but we haven't for non-ALU
> operations. This changes that.
>
> total NIR instructions in shared programs: 2039883 -> 2022338 (-0.86%)
> NIR instructions in affected programs: 1768850 -> 1751305 (-0.99%)
> helped:14244
> HURT: 124
It's great to see these cleaned up - programs were littered with
ssa_1 = vec4(ssa_2.x, ssa_2.y, ssa_2.z, ssa_3.w);
Now they're much better.
> total FS instructions in shared programs: 4083960 -> 4084036 (0.00%)
> FS instructions in affected programs: 7302 -> 7378 (1.04%)
> helped: 12
> HURT: 51
This is strange. At least many programs aren't affected.
Presumably, the backend was cleaning up most of these MOVs anyway, and
tidying up the NIR code will help down the road.
> Signed-off-by: Jason Ekstrand
Reviewed-by: Kenneth Graunke
> ---
> src/glsl/nir/nir_opt_copy_propagate.c | 45
> ++-
> 1 file changed, 29 insertions(+), 16 deletions(-)
>
> diff --git a/src/glsl/nir/nir_opt_copy_propagate.c
> b/src/glsl/nir/nir_opt_copy_propagate.c
> index dd0ec01..ee78e5a 100644
> --- a/src/glsl/nir/nir_opt_copy_propagate.c
> +++ b/src/glsl/nir/nir_opt_copy_propagate.c
> @@ -53,22 +53,6 @@ static bool is_move(nir_alu_instr *instr)
>
> }
>
> -static bool
> -is_swizzleless_move(nir_alu_instr *instr)
> -{
> - if (!is_move(instr))
> - return false;
> -
> - for (unsigned i = 0; i < 4; i++) {
> - if (!((instr->dest.write_mask >> i) & 1))
> - break;
> - if (instr->src[0].swizzle[i] != i)
> - return false;
> - }
> -
> - return true;
> -}
> -
> static bool is_vec(nir_alu_instr *instr)
> {
> for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++)
> @@ -80,6 +64,35 @@ static bool is_vec(nir_alu_instr *instr)
>instr->op == nir_op_vec4;
> }
>
> +static bool
> +is_swizzleless_move(nir_alu_instr *instr)
> +{
> + if (is_move(instr)) {
> + for (unsigned i = 0; i < 4; i++) {
> + if (!((instr->dest.write_mask >> i) & 1))
> +break;
> + if (instr->src[0].swizzle[i] != i)
> +return false;
> + }
> + return true;
> + } else if (is_vec(instr)) {
> + nir_ssa_def *def = NULL;
> + for (unsigned i = 0; i < nir_op_infos[instr->op].num_inputs; i++) {
> + if (instr->src[i].swizzle[0] != i)
> +return false;
> +
> + if (def == NULL) {
> +def = instr->src[i].src.ssa;
> + } else if (instr->src[i].src.ssa != def) {
> +return false;
> + }
> + }
> + return true;
> + } else {
> + return false;
> + }
> +}
> +
> typedef struct {
> nir_ssa_def *def;
> bool found;
>
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
