[2/2] radv/gfx10: correctly determine the number of vertices per primitive

Submitted by Samuel Pitoiset on July 22, 2019, 3:53 p.m.

Details

Message ID 20190722155314.32277-2-samuel.pitoiset@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset July 22, 2019, 3:53 p.m.
For TES as NGG.

Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/amd/vulkan/radv_nir_to_llvm.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
index 336bae28614..6e5a283f923 100644
--- a/src/amd/vulkan/radv_nir_to_llvm.c
+++ b/src/amd/vulkan/radv_nir_to_llvm.c
@@ -112,6 +112,7 @@  struct radv_shader_context {
 	unsigned gs_max_out_vertices;
 	unsigned gs_output_prim;
 
+	unsigned tes_point_mode;
 	unsigned tes_primitive_mode;
 
 	uint32_t tcs_patch_outputs_read;
@@ -3304,7 +3305,6 @@  handle_ngg_outputs_post(struct radv_shader_context *ctx)
 {
 	LLVMBuilderRef builder = ctx->ac.builder;
 	struct ac_build_if_state if_state;
-	unsigned num_vertices = 3;
 	LLVMValueRef tmp;
 
 	assert((ctx->stage == MESA_SHADER_VERTEX ||
@@ -3322,6 +3322,20 @@  handle_ngg_outputs_post(struct radv_shader_context *ctx)
 		ac_unpack_param(&ctx->ac, ctx->gs_vtx_offset[2], 0, 16),
 	};
 
+	/* Determine the number of vertices per primitive. */
+	unsigned num_vertices;
+
+	if (ctx->stage == MESA_SHADER_VERTEX) {
+		num_vertices = 3; /* TODO: optimize for points & lines */
+	} else {
+		if (ctx->tes_point_mode)
+			num_vertices = 1;
+		else if (ctx->tes_primitive_mode == GL_LINES)
+			num_vertices = 2;
+		else
+			num_vertices = 3;
+	}
+
 	/* TODO: streamout */
 
 	/* Copy Primitive IDs from GS threads to the LDS address corresponding
@@ -4435,6 +4449,7 @@  LLVMModuleRef ac_translate_nir_to_llvm(struct ac_llvm_compiler *ac_llvm,
 				ctx.tcs_num_inputs = util_last_bit64(shader_info->info.vs.ls_outputs_written);
 			ctx.tcs_num_patches = get_tcs_num_patches(&ctx);
 		} else if (shaders[i]->info.stage == MESA_SHADER_TESS_EVAL) {
+			ctx.tes_point_mode = shaders[i]->info.tess.point_mode;
 			ctx.tes_primitive_mode = shaders[i]->info.tess.primitive_mode;
 			ctx.abi.load_tess_varyings = load_tes_input;
 			ctx.abi.load_tess_coord = load_tess_coord;

Comments

On Mon, Jul 22, 2019 at 11:49 AM Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
> For TES as NGG.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  src/amd/vulkan/radv_nir_to_llvm.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
> index 336bae28614..6e5a283f923 100644
> --- a/src/amd/vulkan/radv_nir_to_llvm.c
> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> @@ -112,6 +112,7 @@ struct radv_shader_context {
>         unsigned gs_max_out_vertices;
>         unsigned gs_output_prim;
>
> +       unsigned tes_point_mode;
>         unsigned tes_primitive_mode;
>
>         uint32_t tcs_patch_outputs_read;
> @@ -3304,7 +3305,6 @@ handle_ngg_outputs_post(struct radv_shader_context *ctx)
>  {
>         LLVMBuilderRef builder = ctx->ac.builder;
>         struct ac_build_if_state if_state;
> -       unsigned num_vertices = 3;
>         LLVMValueRef tmp;
>
>         assert((ctx->stage == MESA_SHADER_VERTEX ||
> @@ -3322,6 +3322,20 @@ handle_ngg_outputs_post(struct radv_shader_context *ctx)
>                 ac_unpack_param(&ctx->ac, ctx->gs_vtx_offset[2], 0, 16),
>         };
>
> +       /* Determine the number of vertices per primitive. */
> +       unsigned num_vertices;
> +
> +       if (ctx->stage == MESA_SHADER_VERTEX) {
> +               num_vertices = 3; /* TODO: optimize for points & lines */
> +       } else {
> +               if (ctx->tes_point_mode)
> +                       num_vertices = 1;
> +               else if (ctx->tes_primitive_mode == GL_LINES)
> +                       num_vertices = 2;
> +               else
> +                       num_vertices = 3;
> +       }
> +
>         /* TODO: streamout */
>
>         /* Copy Primitive IDs from GS threads to the LDS address corresponding
> @@ -4435,6 +4449,7 @@ LLVMModuleRef ac_translate_nir_to_llvm(struct ac_llvm_compiler *ac_llvm,
>                                 ctx.tcs_num_inputs = util_last_bit64(shader_info->info.vs.ls_outputs_written);
>                         ctx.tcs_num_patches = get_tcs_num_patches(&ctx);
>                 } else if (shaders[i]->info.stage == MESA_SHADER_TESS_EVAL) {
> +                       ctx.tes_point_mode = shaders[i]->info.tess.point_mode;

Drive-by-comment without reading the full context...

What if there's e.g. a GS which produces not-points? This bool will be
set, and the logic above will say num_vertices = 1, which presumably
is bad.

  -ilia

>                         ctx.tes_primitive_mode = shaders[i]->info.tess.primitive_mode;
>                         ctx.abi.load_tess_varyings = load_tes_input;
>                         ctx.abi.load_tess_coord = load_tess_coord;
> --
> 2.22.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Mon, Jul 22, 2019 at 6:01 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> On Mon, Jul 22, 2019 at 11:49 AM Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
> >
> > For TES as NGG.
> >
> > Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> > ---
> >  src/amd/vulkan/radv_nir_to_llvm.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
> > index 336bae28614..6e5a283f923 100644
> > --- a/src/amd/vulkan/radv_nir_to_llvm.c
> > +++ b/src/amd/vulkan/radv_nir_to_llvm.c
> > @@ -112,6 +112,7 @@ struct radv_shader_context {
> >         unsigned gs_max_out_vertices;
> >         unsigned gs_output_prim;
> >
> > +       unsigned tes_point_mode;
> >         unsigned tes_primitive_mode;
> >
> >         uint32_t tcs_patch_outputs_read;
> > @@ -3304,7 +3305,6 @@ handle_ngg_outputs_post(struct radv_shader_context *ctx)
> >  {
> >         LLVMBuilderRef builder = ctx->ac.builder;
> >         struct ac_build_if_state if_state;
> > -       unsigned num_vertices = 3;
> >         LLVMValueRef tmp;
> >
> >         assert((ctx->stage == MESA_SHADER_VERTEX ||
> > @@ -3322,6 +3322,20 @@ handle_ngg_outputs_post(struct radv_shader_context *ctx)
> >                 ac_unpack_param(&ctx->ac, ctx->gs_vtx_offset[2], 0, 16),
> >         };
> >
> > +       /* Determine the number of vertices per primitive. */
> > +       unsigned num_vertices;
> > +
> > +       if (ctx->stage == MESA_SHADER_VERTEX) {
> > +               num_vertices = 3; /* TODO: optimize for points & lines */
> > +       } else {
> > +               if (ctx->tes_point_mode)
> > +                       num_vertices = 1;
> > +               else if (ctx->tes_primitive_mode == GL_LINES)
> > +                       num_vertices = 2;
> > +               else
> > +                       num_vertices = 3;
> > +       }
> > +
> >         /* TODO: streamout */
> >
> >         /* Copy Primitive IDs from GS threads to the LDS address corresponding
> > @@ -4435,6 +4449,7 @@ LLVMModuleRef ac_translate_nir_to_llvm(struct ac_llvm_compiler *ac_llvm,
> >                                 ctx.tcs_num_inputs = util_last_bit64(shader_info->info.vs.ls_outputs_written);
> >                         ctx.tcs_num_patches = get_tcs_num_patches(&ctx);
> >                 } else if (shaders[i]->info.stage == MESA_SHADER_TESS_EVAL) {
> > +                       ctx.tes_point_mode = shaders[i]->info.tess.point_mode;
>
> Drive-by-comment without reading the full context...
>
> What if there's e.g. a GS which produces not-points? This bool will be
> set, and the logic above will say num_vertices = 1, which presumably
> is bad.

The invariant you're probably missing here is that
handle_ngg_outputs_post only gets called if there is no GS.
 (And the gs epilogue does not care about these tessellation variables).

>
>   -ilia
>
> >                         ctx.tes_primitive_mode = shaders[i]->info.tess.primitive_mode;
> >                         ctx.abi.load_tess_varyings = load_tes_input;
> >                         ctx.abi.load_tess_coord = load_tess_coord;
> > --
> > 2.22.0
> >
> > _______________________________________________
> > 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
On 7/22/19 6:01 PM, Ilia Mirkin wrote:
> On Mon, Jul 22, 2019 at 11:49 AM Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
>> For TES as NGG.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>   src/amd/vulkan/radv_nir_to_llvm.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/amd/vulkan/radv_nir_to_llvm.c b/src/amd/vulkan/radv_nir_to_llvm.c
>> index 336bae28614..6e5a283f923 100644
>> --- a/src/amd/vulkan/radv_nir_to_llvm.c
>> +++ b/src/amd/vulkan/radv_nir_to_llvm.c
>> @@ -112,6 +112,7 @@ struct radv_shader_context {
>>          unsigned gs_max_out_vertices;
>>          unsigned gs_output_prim;
>>
>> +       unsigned tes_point_mode;
>>          unsigned tes_primitive_mode;
>>
>>          uint32_t tcs_patch_outputs_read;
>> @@ -3304,7 +3305,6 @@ handle_ngg_outputs_post(struct radv_shader_context *ctx)
>>   {
>>          LLVMBuilderRef builder = ctx->ac.builder;
>>          struct ac_build_if_state if_state;
>> -       unsigned num_vertices = 3;
>>          LLVMValueRef tmp;
>>
>>          assert((ctx->stage == MESA_SHADER_VERTEX ||
>> @@ -3322,6 +3322,20 @@ handle_ngg_outputs_post(struct radv_shader_context *ctx)
>>                  ac_unpack_param(&ctx->ac, ctx->gs_vtx_offset[2], 0, 16),
>>          };
>>
>> +       /* Determine the number of vertices per primitive. */
>> +       unsigned num_vertices;
>> +
>> +       if (ctx->stage == MESA_SHADER_VERTEX) {
>> +               num_vertices = 3; /* TODO: optimize for points & lines */
>> +       } else {
>> +               if (ctx->tes_point_mode)
>> +                       num_vertices = 1;
>> +               else if (ctx->tes_primitive_mode == GL_LINES)
>> +                       num_vertices = 2;
>> +               else
>> +                       num_vertices = 3;
>> +       }
>> +
>>          /* TODO: streamout */
>>
>>          /* Copy Primitive IDs from GS threads to the LDS address corresponding
>> @@ -4435,6 +4449,7 @@ LLVMModuleRef ac_translate_nir_to_llvm(struct ac_llvm_compiler *ac_llvm,
>>                                  ctx.tcs_num_inputs = util_last_bit64(shader_info->info.vs.ls_outputs_written);
>>                          ctx.tcs_num_patches = get_tcs_num_patches(&ctx);
>>                  } else if (shaders[i]->info.stage == MESA_SHADER_TESS_EVAL) {
>> +                       ctx.tes_point_mode = shaders[i]->info.tess.point_mode;
> Drive-by-comment without reading the full context...
>
> What if there's e.g. a GS which produces not-points? This bool will be
> set, and the logic above will say num_vertices = 1, which presumably
> is bad.
With GS, TES is emitted as ES and this function isn't called because 
it's for NGG only.
>
>    -ilia
>
>>                          ctx.tes_primitive_mode = shaders[i]->info.tess.primitive_mode;
>>                          ctx.abi.load_tess_varyings = load_tes_input;
>>                          ctx.abi.load_tess_coord = load_tess_coord;
>> --
>> 2.22.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev