subject:"\[Mesa\-dev\] \[PATCH\] tgsi\: Recognize RET in main for tgsi

Re: [Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

2018-02-13 Thread Jose Fonseca


On 13/02/18 17:57, srol...@vmware.com wrote:

From: Roland Scheidegger 

Shaders coming from dx10 state trackers have a RET before the END.
And the epilog needs to be placed before the RET (otherwise it will
get ignored).
Hence figure out if a RET is in main, in this case we'll place
the epilog there rather than before the END.
(At a closer look, there actually seem to be problems with control
flow in general with output redirection, that would need another
look. It's enough however to fix draw's aa line emulation in some
internal bug - lines tend to be drawn with trivial shaders, moving
either a constant color or a vertex color directly to the output).

v2: add assert so buggy handling of RET in main is detected
---
  src/gallium/auxiliary/tgsi/tgsi_transform.c | 62 +
  1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_transform.c 
b/src/gallium/auxiliary/tgsi/tgsi_transform.c
index ffdad13..a13cf90 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_transform.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_transform.c
@@ -110,6 +110,9 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
  {
 uint procType;
 boolean first_instruction = TRUE;
+   boolean epilog_emitted = FALSE;
+   int cond_stack = 0;
+   int call_stack = 0;
  
 /* input shader */

 struct tgsi_parse_context parse;
@@ -166,22 +169,66 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
   {
  struct tgsi_full_instruction *fullinst
 = 
+unsigned opcode = fullinst->Instruction.Opcode;
  
  if (first_instruction && ctx->prolog) {

 ctx->prolog(ctx);
  }
  
-/* XXX Note: we may also want to look for a main/top-level

- * TGSI_OPCODE_RET instruction in the future.
+/*
+ * XXX Note: we handle the case of ret in main.
+ * However, the output redirections done by transform
+ * have their limits with control flow and will generally
+ * not work correctly. e.g.
+ * if (cond) {
+ *oColor = x;
+ *ret;
+ * }
+ * oColor = y;
+ * end;
+ * If the color output is redirected to a temp and modified
+ * by a transform, this will not work (the oColor assignment
+ * in the conditional will never make it to the actual output).
   */
-if (fullinst->Instruction.Opcode == TGSI_OPCODE_END
-&& ctx->epilog) {
-   /* Emit caller's epilog */
-   ctx->epilog(ctx);
-   /* Emit END */
+if ((opcode == TGSI_OPCODE_END || opcode == TGSI_OPCODE_RET) &&
+ call_stack == 0 && ctx->epilog && !epilog_emitted) {
+   if (opcode == TGSI_OPCODE_RET && cond_stack != 0) {
+  assert(!"transform ignoring RET in main");
+   } else {
+  assert(cond_stack == 0);
+  /* Emit caller's epilog */
+  ctx->epilog(ctx);
+  epilog_emitted = TRUE;
+   }
+   /* Emit END (or RET) */
 ctx->emit_instruction(ctx, fullinst);
  }
  else {
+   switch (opcode) {
+   case TGSI_OPCODE_IF:
+   case TGSI_OPCODE_UIF:
+   case TGSI_OPCODE_SWITCH:
+   case TGSI_OPCODE_BGNLOOP:
+  cond_stack++;
+  break;
+   case TGSI_OPCODE_CAL:
+  call_stack++;
+  break;
+   case TGSI_OPCODE_ENDIF:
+   case TGSI_OPCODE_ENDSWITCH:
+   case TGSI_OPCODE_ENDLOOP:
+  assert(cond_stack > 0);
+  cond_stack--;
+  break;
+   case TGSI_OPCODE_ENDSUB:
+  assert(call_stack > 0);
+  call_stack--;
+  break;
+   case TGSI_OPCODE_BGNSUB:
+   case TGSI_OPCODE_RET:
+   default:
+  break;
+   }
 if (ctx->transform_instruction)
ctx->transform_instruction(ctx, fullinst);
 else
@@ -231,6 +278,7 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
   assert( 0 );
}
 }
+   assert(call_stack == 0);
  
 tgsi_parse_free ();
  



LGTM.  Thanks.

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

[Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

2018-02-13 Thread sroland

From: Roland Scheidegger 

Shaders coming from dx10 state trackers have a RET before the END.
And the epilog needs to be placed before the RET (otherwise it will
get ignored).
Hence figure out if a RET is in main, in this case we'll place
the epilog there rather than before the END.
(At a closer look, there actually seem to be problems with control
flow in general with output redirection, that would need another
look. It's enough however to fix draw's aa line emulation in some
internal bug - lines tend to be drawn with trivial shaders, moving
either a constant color or a vertex color directly to the output).

v2: add assert so buggy handling of RET in main is detected
---
 src/gallium/auxiliary/tgsi/tgsi_transform.c | 62 +
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_transform.c 
b/src/gallium/auxiliary/tgsi/tgsi_transform.c
index ffdad13..a13cf90 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_transform.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_transform.c
@@ -110,6 +110,9 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
 {
uint procType;
boolean first_instruction = TRUE;
+   boolean epilog_emitted = FALSE;
+   int cond_stack = 0;
+   int call_stack = 0;
 
/* input shader */
struct tgsi_parse_context parse;
@@ -166,22 +169,66 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
  {
 struct tgsi_full_instruction *fullinst
= 
+unsigned opcode = fullinst->Instruction.Opcode;
 
 if (first_instruction && ctx->prolog) {
ctx->prolog(ctx);
 }
 
-/* XXX Note: we may also want to look for a main/top-level
- * TGSI_OPCODE_RET instruction in the future.
+/*
+ * XXX Note: we handle the case of ret in main.
+ * However, the output redirections done by transform
+ * have their limits with control flow and will generally
+ * not work correctly. e.g.
+ * if (cond) {
+ *oColor = x;
+ *ret;
+ * }
+ * oColor = y;
+ * end;
+ * If the color output is redirected to a temp and modified
+ * by a transform, this will not work (the oColor assignment
+ * in the conditional will never make it to the actual output).
  */
-if (fullinst->Instruction.Opcode == TGSI_OPCODE_END
-&& ctx->epilog) {
-   /* Emit caller's epilog */
-   ctx->epilog(ctx);
-   /* Emit END */
+if ((opcode == TGSI_OPCODE_END || opcode == TGSI_OPCODE_RET) &&
+ call_stack == 0 && ctx->epilog && !epilog_emitted) {
+   if (opcode == TGSI_OPCODE_RET && cond_stack != 0) {
+  assert(!"transform ignoring RET in main");
+   } else {
+  assert(cond_stack == 0);
+  /* Emit caller's epilog */
+  ctx->epilog(ctx);
+  epilog_emitted = TRUE;
+   }
+   /* Emit END (or RET) */
ctx->emit_instruction(ctx, fullinst);
 }
 else {
+   switch (opcode) {
+   case TGSI_OPCODE_IF:
+   case TGSI_OPCODE_UIF:
+   case TGSI_OPCODE_SWITCH:
+   case TGSI_OPCODE_BGNLOOP:
+  cond_stack++;
+  break;
+   case TGSI_OPCODE_CAL:
+  call_stack++;
+  break;
+   case TGSI_OPCODE_ENDIF:
+   case TGSI_OPCODE_ENDSWITCH:
+   case TGSI_OPCODE_ENDLOOP:
+  assert(cond_stack > 0);
+  cond_stack--;
+  break;
+   case TGSI_OPCODE_ENDSUB:
+  assert(call_stack > 0);
+  call_stack--;
+  break;
+   case TGSI_OPCODE_BGNSUB:
+   case TGSI_OPCODE_RET:
+   default:
+  break;
+   }
if (ctx->transform_instruction)
   ctx->transform_instruction(ctx, fullinst);
else
@@ -231,6 +278,7 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
  assert( 0 );
   }
}
+   assert(call_stack == 0);
 
tgsi_parse_free ();
 
-- 
2.7.4

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

Re: [Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

2018-02-13 Thread Jose Fonseca


It looks good to me.

Though I think it would be useful to track nesting of subroutines and 
other control flow separately, and throw a warning when we ignore RETs 
on the main subroutine that are not on the top-most level of the control 
flow stack.  So it's easier to spot when that problem surfaces.


Reviewed-by: Jose Fonseca 


On 13/02/18 04:10, srol...@vmware.com wrote:

From: Roland Scheidegger 

Shaders coming from dx10 state trackers have a RET before the END.
And the epilog needs to be placed before the RET (otherwise it will
get ignored).
Hence figure out if a RET is in main, in this case we'll place
the epilog there rather than before the END.
(At a closer look, there actually seem to be problems with control
flow in general with output redirection, that would need another
look. It's enough however to fix draw's aa line emulation in some
internal bug - lines tend to be drawn with trivial shaders, moving
either a constant color or a vertex color directly to the output).
---
  src/gallium/auxiliary/tgsi/tgsi_transform.c | 50 ++---
  1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_transform.c 
b/src/gallium/auxiliary/tgsi/tgsi_transform.c
index ffdad13..94d872c 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_transform.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_transform.c
@@ -110,6 +110,8 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
  {
 uint procType;
 boolean first_instruction = TRUE;
+   boolean epilog_emitted = FALSE;
+   int stack_size = 0;
  
 /* input shader */

 struct tgsi_parse_context parse;
@@ -166,22 +168,60 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
   {
  struct tgsi_full_instruction *fullinst
 = 
+unsigned opcode = fullinst->Instruction.Opcode;
  
  if (first_instruction && ctx->prolog) {

 ctx->prolog(ctx);
  }
  
-/* XXX Note: we may also want to look for a main/top-level

- * TGSI_OPCODE_RET instruction in the future.
+/*
+ * XXX Note: we handle the case of ret in main.
+ * However, the output redirections done by transform
+ * have their limits with control flow and will generally
+ * not work correctly. e.g.
+ * if (cond) {
+ *oColor = x;
+ *ret;
+ * }
+ * oColor = y;
+ * end;
+ * If the color output is redirected to a temp and modified
+ * by a transform, this will not work (the oColor assignment
+ * in the conditional will never make it to the actual output).
   */
-if (fullinst->Instruction.Opcode == TGSI_OPCODE_END
-&& ctx->epilog) {
+if ((opcode == TGSI_OPCODE_END ||
+ (opcode == TGSI_OPCODE_RET && stack_size == 0))
+&& ctx->epilog && !epilog_emitted) {
 /* Emit caller's epilog */
 ctx->epilog(ctx);
-   /* Emit END */
+   epilog_emitted = TRUE;
+   /* Emit END (or RET) */
+   if (opcode == TGSI_OPCODE_END) {
+  assert(stack_size == 0);
+   }
 ctx->emit_instruction(ctx, fullinst);
  }
  else {
+   switch (opcode) {
+   case TGSI_OPCODE_IF:
+   case TGSI_OPCODE_UIF:
+   case TGSI_OPCODE_SWITCH:
+   case TGSI_OPCODE_BGNLOOP:
+   case TGSI_OPCODE_CAL:
+  stack_size++;
+  break;
+   case TGSI_OPCODE_ENDIF:
+   case TGSI_OPCODE_ENDSWITCH:
+   case TGSI_OPCODE_ENDLOOP:
+   case TGSI_OPCODE_ENDSUB:
+  assert(stack_size > 0);
+  stack_size--;
+  break;
+   case TGSI_OPCODE_BGNSUB:
+   case TGSI_OPCODE_RET:
+   default:
+  break;
+   }
 if (ctx->transform_instruction)
ctx->transform_instruction(ctx, fullinst);
 else



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

Re: [Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

2018-02-12 Thread Brian Paul


LGTM.

Reviewed-by: Brian Paul 


On 02/12/2018 09:10 PM, srol...@vmware.com wrote:

From: Roland Scheidegger 

Shaders coming from dx10 state trackers have a RET before the END.
And the epilog needs to be placed before the RET (otherwise it will
get ignored).
Hence figure out if a RET is in main, in this case we'll place
the epilog there rather than before the END.
(At a closer look, there actually seem to be problems with control
flow in general with output redirection, that would need another
look. It's enough however to fix draw's aa line emulation in some
internal bug - lines tend to be drawn with trivial shaders, moving
either a constant color or a vertex color directly to the output).
---
  src/gallium/auxiliary/tgsi/tgsi_transform.c | 50 ++---
  1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_transform.c 
b/src/gallium/auxiliary/tgsi/tgsi_transform.c
index ffdad13..94d872c 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_transform.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_transform.c
@@ -110,6 +110,8 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
  {
 uint procType;
 boolean first_instruction = TRUE;
+   boolean epilog_emitted = FALSE;
+   int stack_size = 0;
  
 /* input shader */

 struct tgsi_parse_context parse;
@@ -166,22 +168,60 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
   {
  struct tgsi_full_instruction *fullinst
 = 
+unsigned opcode = fullinst->Instruction.Opcode;
  
  if (first_instruction && ctx->prolog) {

 ctx->prolog(ctx);
  }
  
-/* XXX Note: we may also want to look for a main/top-level

- * TGSI_OPCODE_RET instruction in the future.
+/*
+ * XXX Note: we handle the case of ret in main.
+ * However, the output redirections done by transform
+ * have their limits with control flow and will generally
+ * not work correctly. e.g.
+ * if (cond) {
+ *oColor = x;
+ *ret;
+ * }
+ * oColor = y;
+ * end;
+ * If the color output is redirected to a temp and modified
+ * by a transform, this will not work (the oColor assignment
+ * in the conditional will never make it to the actual output).
   */
-if (fullinst->Instruction.Opcode == TGSI_OPCODE_END
-&& ctx->epilog) {
+if ((opcode == TGSI_OPCODE_END ||
+ (opcode == TGSI_OPCODE_RET && stack_size == 0))
+&& ctx->epilog && !epilog_emitted) {
 /* Emit caller's epilog */
 ctx->epilog(ctx);
-   /* Emit END */
+   epilog_emitted = TRUE;
+   /* Emit END (or RET) */
+   if (opcode == TGSI_OPCODE_END) {
+  assert(stack_size == 0);
+   }
 ctx->emit_instruction(ctx, fullinst);
  }
  else {
+   switch (opcode) {
+   case TGSI_OPCODE_IF:
+   case TGSI_OPCODE_UIF:
+   case TGSI_OPCODE_SWITCH:
+   case TGSI_OPCODE_BGNLOOP:
+   case TGSI_OPCODE_CAL:
+  stack_size++;
+  break;
+   case TGSI_OPCODE_ENDIF:
+   case TGSI_OPCODE_ENDSWITCH:
+   case TGSI_OPCODE_ENDLOOP:
+   case TGSI_OPCODE_ENDSUB:
+  assert(stack_size > 0);
+  stack_size--;
+  break;
+   case TGSI_OPCODE_BGNSUB:
+   case TGSI_OPCODE_RET:
+   default:
+  break;
+   }
 if (ctx->transform_instruction)
ctx->transform_instruction(ctx, fullinst);
 else



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

[Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

2018-02-12 Thread sroland

From: Roland Scheidegger 

Shaders coming from dx10 state trackers have a RET before the END.
And the epilog needs to be placed before the RET (otherwise it will
get ignored).
Hence figure out if a RET is in main, in this case we'll place
the epilog there rather than before the END.
(At a closer look, there actually seem to be problems with control
flow in general with output redirection, that would need another
look. It's enough however to fix draw's aa line emulation in some
internal bug - lines tend to be drawn with trivial shaders, moving
either a constant color or a vertex color directly to the output).
---
 src/gallium/auxiliary/tgsi/tgsi_transform.c | 50 ++---
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_transform.c 
b/src/gallium/auxiliary/tgsi/tgsi_transform.c
index ffdad13..94d872c 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_transform.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_transform.c
@@ -110,6 +110,8 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
 {
uint procType;
boolean first_instruction = TRUE;
+   boolean epilog_emitted = FALSE;
+   int stack_size = 0;
 
/* input shader */
struct tgsi_parse_context parse;
@@ -166,22 +168,60 @@ tgsi_transform_shader(const struct tgsi_token *tokens_in,
  {
 struct tgsi_full_instruction *fullinst
= 
+unsigned opcode = fullinst->Instruction.Opcode;
 
 if (first_instruction && ctx->prolog) {
ctx->prolog(ctx);
 }
 
-/* XXX Note: we may also want to look for a main/top-level
- * TGSI_OPCODE_RET instruction in the future.
+/*
+ * XXX Note: we handle the case of ret in main.
+ * However, the output redirections done by transform
+ * have their limits with control flow and will generally
+ * not work correctly. e.g.
+ * if (cond) {
+ *oColor = x;
+ *ret;
+ * }
+ * oColor = y;
+ * end;
+ * If the color output is redirected to a temp and modified
+ * by a transform, this will not work (the oColor assignment
+ * in the conditional will never make it to the actual output).
  */
-if (fullinst->Instruction.Opcode == TGSI_OPCODE_END
-&& ctx->epilog) {
+if ((opcode == TGSI_OPCODE_END ||
+ (opcode == TGSI_OPCODE_RET && stack_size == 0))
+&& ctx->epilog && !epilog_emitted) {
/* Emit caller's epilog */
ctx->epilog(ctx);
-   /* Emit END */
+   epilog_emitted = TRUE;
+   /* Emit END (or RET) */
+   if (opcode == TGSI_OPCODE_END) {
+  assert(stack_size == 0);
+   }
ctx->emit_instruction(ctx, fullinst);
 }
 else {
+   switch (opcode) {
+   case TGSI_OPCODE_IF:
+   case TGSI_OPCODE_UIF:
+   case TGSI_OPCODE_SWITCH:
+   case TGSI_OPCODE_BGNLOOP:
+   case TGSI_OPCODE_CAL:
+  stack_size++;
+  break;
+   case TGSI_OPCODE_ENDIF:
+   case TGSI_OPCODE_ENDSWITCH:
+   case TGSI_OPCODE_ENDLOOP:
+   case TGSI_OPCODE_ENDSUB:
+  assert(stack_size > 0);
+  stack_size--;
+  break;
+   case TGSI_OPCODE_BGNSUB:
+   case TGSI_OPCODE_RET:
+   default:
+  break;
+   }
if (ctx->transform_instruction)
   ctx->transform_instruction(ctx, fullinst);
else
-- 
2.7.4

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

Re: [Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

[Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

Re: [Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

Re: [Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

[Mesa-dev] [PATCH] tgsi: Recognize RET in main for tgsi_transform

5 matches

Site Navigation

Mail list logo

Footer information