Re: [Mesa-dev] [PATCH] radeonsi: fix an off-by-one error in the bounds check for max_vertices

[Marek Olšák]

Reviewed-by: Marek Olšák 

Marek

On Tue, Dec 6, 2016 at 9:06 PM, Nicolai Hähnle  wrote:
> From: Nicolai Hähnle 
>
> The spec actually says that calling EmitStreamVertex is undefined when
> you exceed max_vertices. But we do need to avoid trampling over memory
> outside the GSVS ring.
>
> Cc: mesa-sta...@lists.freedesktop.org
> --
> One more thing I noticed on top of all the other GS changes...
> ---
>  src/gallium/drivers/radeonsi/si_shader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c 
> b/src/gallium/drivers/radeonsi/si_shader.c
> index dee2a17..749823d 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -5181,21 +5181,21 @@ static void si_llvm_emit_vertex(
>"");
>
> /* If this thread has already emitted the declared maximum number of
>  * vertices, skip the write: excessive vertex emissions are not
>  * supposed to have any effect.
>  *
>  * If the shader has no writes to memory, kill it instead. This skips
>  * further memory loads and may allow LLVM to skip to the end
>  * altogether.
>  */
> -   can_emit = LLVMBuildICmp(gallivm->builder, LLVMIntULE, gs_next_vertex,
> +   can_emit = LLVMBuildICmp(gallivm->builder, LLVMIntULT, gs_next_vertex,
>  lp_build_const_int32(gallivm,
>   
> shader->selector->gs_max_out_vertices), "");
>
> bool use_kill = !info->writes_memory;
> if (use_kill) {
> kill = lp_build_select(_base->base, can_emit,
>lp_build_const_float(gallivm, 1.0f),
>lp_build_const_float(gallivm, -1.0f));
>
> lp_build_intrinsic(gallivm->builder, "llvm.AMDGPU.kill",
> --
> 2.7.4
>
> ___
> 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] radeonsi: fix an off-by-one error in the bounds check for max_vertices

[Nicolai Hähnle]

On 07.12.2016 04:47, Michel Dänzer wrote:

On 07/12/16 05:06 AM, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

The spec actually says that calling EmitStreamVertex is undefined when
you exceed max_vertices. But we do need to avoid trampling over memory
outside the GSVS ring.


Why "but"? It still works up to max_vertices with your fix, right? Anyway,


When the spec says "in situation XYZ, behavior is undefined", you often 
don't actually have to check whether situation XYZ occurs at all. In 
that sense "but": undefined behavior in OpenGL usually isn't meant to 
include crashes or GPU hangs, which could happen from writing outside of 
buffer bounds. So we do need to check.




Reviewed-by: Michel Dänzer 


Thanks!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: fix an off-by-one error in the bounds check for max_vertices

[Michel Dänzer]

On 07/12/16 05:06 AM, Nicolai Hähnle wrote:
> From: Nicolai Hähnle 
> 
> The spec actually says that calling EmitStreamVertex is undefined when
> you exceed max_vertices. But we do need to avoid trampling over memory
> outside the GSVS ring.

Why "but"? It still works up to max_vertices with your fix, right? Anyway,

Reviewed-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: fix an off-by-one error in the bounds check for max_vertices

[Edward O'Callaghan]

Reviewed-by: Edward O'Callaghan 

On 12/07/2016 07:06 AM, Nicolai Hähnle wrote:
> From: Nicolai Hähnle 
> 
> The spec actually says that calling EmitStreamVertex is undefined when
> you exceed max_vertices. But we do need to avoid trampling over memory
> outside the GSVS ring.
> 
> Cc: mesa-sta...@lists.freedesktop.org
> --
> One more thing I noticed on top of all the other GS changes...
> ---
>  src/gallium/drivers/radeonsi/si_shader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c 
> b/src/gallium/drivers/radeonsi/si_shader.c
> index dee2a17..749823d 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -5181,21 +5181,21 @@ static void si_llvm_emit_vertex(
>  "");
>  
>   /* If this thread has already emitted the declared maximum number of
>* vertices, skip the write: excessive vertex emissions are not
>* supposed to have any effect.
>*
>* If the shader has no writes to memory, kill it instead. This skips
>* further memory loads and may allow LLVM to skip to the end
>* altogether.
>*/
> - can_emit = LLVMBuildICmp(gallivm->builder, LLVMIntULE, gs_next_vertex,
> + can_emit = LLVMBuildICmp(gallivm->builder, LLVMIntULT, gs_next_vertex,
>lp_build_const_int32(gallivm,
> 
> shader->selector->gs_max_out_vertices), "");
>  
>   bool use_kill = !info->writes_memory;
>   if (use_kill) {
>   kill = lp_build_select(_base->base, can_emit,
>  lp_build_const_float(gallivm, 1.0f),
>  lp_build_const_float(gallivm, -1.0f));
>  
>   lp_build_intrinsic(gallivm->builder, "llvm.AMDGPU.kill",
> 



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radeonsi: fix an off-by-one error in the bounds check for max_vertices

[Nicolai Hähnle]

From: Nicolai Hähnle 

The spec actually says that calling EmitStreamVertex is undefined when
you exceed max_vertices. But we do need to avoid trampling over memory
outside the GSVS ring.

Cc: mesa-sta...@lists.freedesktop.org
--
One more thing I noticed on top of all the other GS changes...
---
 src/gallium/drivers/radeonsi/si_shader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/radeonsi/si_shader.c 
b/src/gallium/drivers/radeonsi/si_shader.c
index dee2a17..749823d 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -5181,21 +5181,21 @@ static void si_llvm_emit_vertex(
   "");
 
/* If this thread has already emitted the declared maximum number of
 * vertices, skip the write: excessive vertex emissions are not
 * supposed to have any effect.
 *
 * If the shader has no writes to memory, kill it instead. This skips
 * further memory loads and may allow LLVM to skip to the end
 * altogether.
 */
-   can_emit = LLVMBuildICmp(gallivm->builder, LLVMIntULE, gs_next_vertex,
+   can_emit = LLVMBuildICmp(gallivm->builder, LLVMIntULT, gs_next_vertex,
 lp_build_const_int32(gallivm,
  
shader->selector->gs_max_out_vertices), "");
 
bool use_kill = !info->writes_memory;
if (use_kill) {
kill = lp_build_select(_base->base, can_emit,
   lp_build_const_float(gallivm, 1.0f),
   lp_build_const_float(gallivm, -1.0f));
 
lp_build_intrinsic(gallivm->builder, "llvm.AMDGPU.kill",
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev