Re: [Mesa-dev] [PATCH 2/3] i965/vec4: Don't fix_math_operand() on Gen >= 8.

2014-06-24 Thread Matt Turner 
On Mon, Jun 23, 2014 at 7:36 PM, Ben Widawsky  wrote:
> On Mon, Jun 23, 2014 at 01:30:14PM -0700, Matt Turner wrote:
>> The emit_math?_gen? functions serve to implement workarounds for the
>> math instruction, none of which exist on Gen8+.
>
> There are still several restrictions with the math instruction on gen8.
> Looking at the existing code, I realize I do not quite see even all the
> gen7 restrictions, so I am not qualified to determine if you accounted
> for gen8.

We don't represent some of these at this level, like vertical stride.
When we're assembling instructions later (in brw_eu_emit.c:gen6_math
and gen8_generator.cpp:gen8_generator::math which should be going away
soon) we have a bunch of assertions that verify these restrictions. It
looks like you're right that we don't have asserts to check the Gen7
restrictions, and our asserts in the Gen8 code seem to be lacking the
check you mentioned below.

> In particular, stride of src, and dest must be the same, and
> "Regioning must ensure Src.Vstride = Src.Width * Src.Hstride"
>
> The rest of the series is:
> Reviewed-by: Ben Widawsky 
>
> If you can convince me these cases are solid here, this one is also r-b
>
> Thanks. (sorry for my ignorance)

Thanks for the review and the good question.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/3] i965/vec4: Don't fix_math_operand() on Gen >= 8.

2014-06-23 Thread Ben Widawsky 
On Mon, Jun 23, 2014 at 01:30:14PM -0700, Matt Turner wrote:
> The emit_math?_gen? functions serve to implement workarounds for the
> math instruction, none of which exist on Gen8+.

There are still several restrictions with the math instruction on gen8.
Looking at the existing code, I realize I do not quite see even all the
gen7 restrictions, so I am not qualified to determine if you accounted
for gen8.

In particular, stride of src, and dest must be the same, and
"Regioning must ensure Src.Vstride = Src.Width * Src.Hstride"

The rest of the series is:
Reviewed-by: Ben Widawsky 

If you can convince me these cases are solid here, this one is also r-b

Thanks. (sorry for my ignorance)

> ---
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp 
> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index f3316f8..3a360d4 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -365,7 +365,9 @@ vec4_visitor::emit_math(opcode opcode, dst_reg dst, 
> src_reg src)
>return;
> }
>  
> -   if (brw->gen >= 6) {
> +   if (brw->gen >= 8) {
> +  emit(opcode, dst, src);
> +   } else if (brw->gen >= 6) {
>emit_math1_gen6(opcode, dst, src);
> } else {
>emit_math1_gen4(opcode, dst, src);
> @@ -417,7 +419,9 @@ vec4_visitor::emit_math(enum opcode opcode,
>return;
> }
>  
> -   if (brw->gen >= 6) {
> +   if (brw->gen >= 8) {
> +  emit(opcode, dst, src0, src1);
> +   } else if (brw->gen >= 6) {
>emit_math2_gen6(opcode, dst, src0, src1);
> } else {
>emit_math2_gen4(opcode, dst, src0, src1);
> -- 
> 1.8.3.2
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/3] i965/vec4: Don't fix_math_operand() on Gen >= 8.

2014-06-23 Thread Matt Turner 
The emit_math?_gen? functions serve to implement workarounds for the
math instruction, none of which exist on Gen8+.
---
 src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp 
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index f3316f8..3a360d4 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -365,7 +365,9 @@ vec4_visitor::emit_math(opcode opcode, dst_reg dst, src_reg 
src)
   return;
}
 
-   if (brw->gen >= 6) {
+   if (brw->gen >= 8) {
+  emit(opcode, dst, src);
+   } else if (brw->gen >= 6) {
   emit_math1_gen6(opcode, dst, src);
} else {
   emit_math1_gen4(opcode, dst, src);
@@ -417,7 +419,9 @@ vec4_visitor::emit_math(enum opcode opcode,
   return;
}
 
-   if (brw->gen >= 6) {
+   if (brw->gen >= 8) {
+  emit(opcode, dst, src0, src1);
+   } else if (brw->gen >= 6) {
   emit_math2_gen6(opcode, dst, src0, src1);
} else {
   emit_math2_gen4(opcode, dst, src0, src1);
-- 
1.8.3.2

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