Re: [Mesa-dev] [PATCH] draw: initialize shader inputs

[Roland Scheidegger]

Am 12.10.2016 um 18:12 schrieb Marek Olšák:
> On Wed, Oct 12, 2016 at 12:04 AM,   wrote:
>> From: Roland Scheidegger 
>>
>> This should make the code more robust if a shader tries to use inputs which
>> aren't defined by the vertex element layout (which usually shouldn't happen).
>>
>> No piglit change.
>> ---
>>  src/gallium/auxiliary/draw/draw_llvm.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c 
>> b/src/gallium/auxiliary/draw/draw_llvm.c
>> index 87951fa..4270a8f 100644
>> --- a/src/gallium/auxiliary/draw/draw_llvm.c
>> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
>> @@ -1705,6 +1705,13 @@ draw_llvm_generate(struct draw_llvm *llvm, struct 
>> draw_llvm_variant *variant,
>>lp_build_printf(gallivm, " --- io %d = %p, loop counter %d\n",
>>io_itr, io, lp_loop.counter);
>>  #endif
>> +
>> +  for (j = draw->pt.nr_vertex_elements; j < PIPE_MAX_SHADER_INPUTS; 
>> j++) {
>> + for (i = 0; i < TGSI_NUM_CHANNELS; i++) {
>> +inputs[j][i] = lp_build_zero(gallivm, vs_type);
>> + }
>> +  }
> 
> It's better to use LLVMGetUndef for uninitialized inputs and temps.
> That can eliminate more code than initializing everything to 0.
> (radeonsi uses undef)

Yes, that's true. I used zeros on purpose though for dx10 needs, albeit
I'm not sure if it's strictly necessary actually (inputs where there's
no buffer bound need to read as zero, however in this case here there's
actually a non-matching input layout, I'm not entirely sure this also
really needs to return zeros). In any case, it shouldn't usually matter
(as the additional code should disappear completely).

Roland

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

Re: [Mesa-dev] [PATCH] draw: initialize shader inputs

[Marek Olšák]

On Wed, Oct 12, 2016 at 12:04 AM,   wrote:
> From: Roland Scheidegger 
>
> This should make the code more robust if a shader tries to use inputs which
> aren't defined by the vertex element layout (which usually shouldn't happen).
>
> No piglit change.
> ---
>  src/gallium/auxiliary/draw/draw_llvm.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c 
> b/src/gallium/auxiliary/draw/draw_llvm.c
> index 87951fa..4270a8f 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
> @@ -1705,6 +1705,13 @@ draw_llvm_generate(struct draw_llvm *llvm, struct 
> draw_llvm_variant *variant,
>lp_build_printf(gallivm, " --- io %d = %p, loop counter %d\n",
>io_itr, io, lp_loop.counter);
>  #endif
> +
> +  for (j = draw->pt.nr_vertex_elements; j < PIPE_MAX_SHADER_INPUTS; j++) 
> {
> + for (i = 0; i < TGSI_NUM_CHANNELS; i++) {
> +inputs[j][i] = lp_build_zero(gallivm, vs_type);
> + }
> +  }

It's better to use LLVMGetUndef for uninitialized inputs and temps.
That can eliminate more code than initializing everything to 0.
(radeonsi uses undef)

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

Re: [Mesa-dev] [PATCH] draw: initialize shader inputs

[Jose Fonseca]

On 11/10/16 23:04, srol...@vmware.com wrote:

From: Roland Scheidegger 

This should make the code more robust if a shader tries to use inputs which
aren't defined by the vertex element layout (which usually shouldn't happen).

No piglit change.
---
 src/gallium/auxiliary/draw/draw_llvm.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/gallium/auxiliary/draw/draw_llvm.c 
b/src/gallium/auxiliary/draw/draw_llvm.c
index 87951fa..4270a8f 100644
--- a/src/gallium/auxiliary/draw/draw_llvm.c
+++ b/src/gallium/auxiliary/draw/draw_llvm.c
@@ -1705,6 +1705,13 @@ draw_llvm_generate(struct draw_llvm *llvm, struct 
draw_llvm_variant *variant,
   lp_build_printf(gallivm, " --- io %d = %p, loop counter %d\n",
   io_itr, io, lp_loop.counter);
 #endif
+
+  for (j = draw->pt.nr_vertex_elements; j < PIPE_MAX_SHADER_INPUTS; j++) {
+ for (i = 0; i < TGSI_NUM_CHANNELS; i++) {
+inputs[j][i] = lp_build_zero(gallivm, vs_type);
+ }
+  }
+
   for (i = 0; i < vector_length; ++i) {
  LLVMValueRef vert_index =
 LLVMBuildAdd(builder,



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

Re: [Mesa-dev] [PATCH] draw: initialize shader inputs

[Roland Scheidegger]

Am 12.10.2016 um 02:21 schrieb Kenneth Graunke:
> On Wednesday, October 12, 2016 12:04:34 AM PDT srol...@vmware.com wrote:
>> From: Roland Scheidegger 
>>
>> This should make the code more robust if a shader tries to use inputs which
>> aren't defined by the vertex element layout (which usually shouldn't happen).
>>
>> No piglit change.
>> ---
>>  src/gallium/auxiliary/draw/draw_llvm.c | 7 +++
>>  1 file changed, 7 insertions(+)
> 
> Well, "robust", or "hiding application bugs"...this likely introduces a
> small but non-zero cost for correctly written applications, and only
> benefits broken ones...seems like a bad trade-off to me...
> 
> For shader temporaries, we made this a drirc option.

Well, typically we'd like to operate with dx10 rules in mind, which
mandate "do not crash" (and more). (llvmpipe generally honors everything
robust_buffer_access, robust_buffer_access_behavior would require, and
sometimes actually at a significant cost - the cost here should be very
minimal since llvm can deduce these inputs aren't actually used (with
correct apps) and throw the code out, the assembly will stay the same).
Though I'm not sure this case can actually be hit with mesa/st, even
with incorrect apps...

If you'd really like to have faster, but not robust llvmpipe, you could
make it configurable - I assure you there's way more bigger fish to fry
than this particular bit (even in draw).

Roland

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

Re: [Mesa-dev] [PATCH] draw: initialize shader inputs

[Kenneth Graunke]

On Wednesday, October 12, 2016 12:04:34 AM PDT srol...@vmware.com wrote:
> From: Roland Scheidegger 
> 
> This should make the code more robust if a shader tries to use inputs which
> aren't defined by the vertex element layout (which usually shouldn't happen).
> 
> No piglit change.
> ---
>  src/gallium/auxiliary/draw/draw_llvm.c | 7 +++
>  1 file changed, 7 insertions(+)

Well, "robust", or "hiding application bugs"...this likely introduces a
small but non-zero cost for correctly written applications, and only
benefits broken ones...seems like a bad trade-off to me...

For shader temporaries, we made this a drirc option.

--Ken


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] draw: initialize shader inputs

[Brian Paul]

On 10/11/2016 04:04 PM, srol...@vmware.com wrote:

From: Roland Scheidegger 

This should make the code more robust if a shader tries to use inputs which
aren't defined by the vertex element layout (which usually shouldn't happen).

No piglit change.
---
  src/gallium/auxiliary/draw/draw_llvm.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/src/gallium/auxiliary/draw/draw_llvm.c 
b/src/gallium/auxiliary/draw/draw_llvm.c
index 87951fa..4270a8f 100644
--- a/src/gallium/auxiliary/draw/draw_llvm.c
+++ b/src/gallium/auxiliary/draw/draw_llvm.c
@@ -1705,6 +1705,13 @@ draw_llvm_generate(struct draw_llvm *llvm, struct 
draw_llvm_variant *variant,
lp_build_printf(gallivm, " --- io %d = %p, loop counter %d\n",
io_itr, io, lp_loop.counter);
  #endif
+
+  for (j = draw->pt.nr_vertex_elements; j < PIPE_MAX_SHADER_INPUTS; j++) {
+ for (i = 0; i < TGSI_NUM_CHANNELS; i++) {
+inputs[j][i] = lp_build_zero(gallivm, vs_type);
+ }
+  }
+
for (i = 0; i < vector_length; ++i) {
   LLVMValueRef vert_index =
  LLVMBuildAdd(builder,



Reviewed-by: Brian Paul 

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