[1/1] ac/nir: use ordered float comparisons except for not equal

Submitted by Samuel Pitoiset on Feb. 23, 2018, 1:21 p.m.

Details

Message ID 20180223132106.14645-2-samuel.pitoiset@gmail.com
State Accepted
Commit e05507a427b79e481eb8e45d7aa3c9b4b78376bf
Headers show
Series "radv: one fix for Wolfenstein 2" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset Feb. 23, 2018, 1:21 p.m.
Original patch from Timothy Arceri, I have just fixed the
not equal case locally.

This fixes one important rendering issue in Wolfenstein 2
(the cutscene transition issue).

RadeonSI uses the same ordered comparisons, so I guess that
what we should do as well.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104302
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104905
Cc: <mesa-stable@lists.freedesktop.org>
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/amd/common/ac_nir_to_llvm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
index cccc687157..bc1d16d2a4 100644
--- a/src/amd/common/ac_nir_to_llvm.c
+++ b/src/amd/common/ac_nir_to_llvm.c
@@ -1801,16 +1801,16 @@  static void visit_alu(struct ac_nir_context *ctx, const nir_alu_instr *instr)
 		result = emit_int_cmp(&ctx->ac, LLVMIntUGE, src[0], src[1]);
 		break;
 	case nir_op_feq:
-		result = emit_float_cmp(&ctx->ac, LLVMRealUEQ, src[0], src[1]);
+		result = emit_float_cmp(&ctx->ac, LLVMRealOEQ, src[0], src[1]);
 		break;
 	case nir_op_fne:
 		result = emit_float_cmp(&ctx->ac, LLVMRealUNE, src[0], src[1]);
 		break;
 	case nir_op_flt:
-		result = emit_float_cmp(&ctx->ac, LLVMRealULT, src[0], src[1]);
+		result = emit_float_cmp(&ctx->ac, LLVMRealOLT, src[0], src[1]);
 		break;
 	case nir_op_fge:
-		result = emit_float_cmp(&ctx->ac, LLVMRealUGE, src[0], src[1]);
+		result = emit_float_cmp(&ctx->ac, LLVMRealOGE, src[0], src[1]);
 		break;
 	case nir_op_fabs:
 		result = emit_intrin_1f_param(&ctx->ac, "llvm.fabs",

Comments

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek

On Fri, Feb 23, 2018 at 2:21 PM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
> Original patch from Timothy Arceri, I have just fixed the
> not equal case locally.
>
> This fixes one important rendering issue in Wolfenstein 2
> (the cutscene transition issue).
>
> RadeonSI uses the same ordered comparisons, so I guess that
> what we should do as well.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104302
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104905
> Cc: <mesa-stable@lists.freedesktop.org>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  src/amd/common/ac_nir_to_llvm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
> index cccc687157..bc1d16d2a4 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -1801,16 +1801,16 @@ static void visit_alu(struct ac_nir_context *ctx, const nir_alu_instr *instr)
>                 result = emit_int_cmp(&ctx->ac, LLVMIntUGE, src[0], src[1]);
>                 break;
>         case nir_op_feq:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealUEQ, src[0], src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOEQ, src[0], src[1]);
>                 break;
>         case nir_op_fne:
>                 result = emit_float_cmp(&ctx->ac, LLVMRealUNE, src[0], src[1]);
>                 break;
>         case nir_op_flt:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealULT, src[0], src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOLT, src[0], src[1]);
>                 break;
>         case nir_op_fge:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealUGE, src[0], src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOGE, src[0], src[1]);
>                 break;
>         case nir_op_fabs:
>                 result = emit_intrin_1f_param(&ctx->ac, "llvm.fabs",
> --
> 2.16.2
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
Yes, this is the right ordering for NIR (and GLSL) comparisons.

Reviewed-by: Connor Abbott <cwabbott0@gmail.com>

On Fri, Feb 23, 2018 at 8:21 AM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
> Original patch from Timothy Arceri, I have just fixed the
> not equal case locally.
>
> This fixes one important rendering issue in Wolfenstein 2
> (the cutscene transition issue).
>
> RadeonSI uses the same ordered comparisons, so I guess that
> what we should do as well.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104302
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104905
> Cc: <mesa-stable@lists.freedesktop.org>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  src/amd/common/ac_nir_to_llvm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
> index cccc687157..bc1d16d2a4 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -1801,16 +1801,16 @@ static void visit_alu(struct ac_nir_context *ctx, const nir_alu_instr *instr)
>                 result = emit_int_cmp(&ctx->ac, LLVMIntUGE, src[0], src[1]);
>                 break;
>         case nir_op_feq:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealUEQ, src[0], src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOEQ, src[0], src[1]);
>                 break;
>         case nir_op_fne:
>                 result = emit_float_cmp(&ctx->ac, LLVMRealUNE, src[0], src[1]);
>                 break;
>         case nir_op_flt:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealULT, src[0], src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOLT, src[0], src[1]);
>                 break;
>         case nir_op_fge:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealUGE, src[0], src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOGE, src[0], src[1]);
>                 break;
>         case nir_op_fabs:
>                 result = emit_intrin_1f_param(&ctx->ac, "llvm.fabs",
> --
> 2.16.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Using this on its own I believe will cause CTS regressions, which is 
what the other patches were about. Feel free to take on the feedback and 
come up with a proper solution. I'm not really sure how to progress this.


On 24/02/18 00:21, Samuel Pitoiset wrote:
> Original patch from Timothy Arceri, I have just fixed the
> not equal case locally.
> 
> This fixes one important rendering issue in Wolfenstein 2
> (the cutscene transition issue).
> 
> RadeonSI uses the same ordered comparisons, so I guess that
> what we should do as well.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104302
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104905
> Cc: <mesa-stable@lists.freedesktop.org>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>   src/amd/common/ac_nir_to_llvm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
> index cccc687157..bc1d16d2a4 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -1801,16 +1801,16 @@ static void visit_alu(struct ac_nir_context *ctx, const nir_alu_instr *instr)
>   		result = emit_int_cmp(&ctx->ac, LLVMIntUGE, src[0], src[1]);
>   		break;
>   	case nir_op_feq:
> -		result = emit_float_cmp(&ctx->ac, LLVMRealUEQ, src[0], src[1]);
> +		result = emit_float_cmp(&ctx->ac, LLVMRealOEQ, src[0], src[1]);
>   		break;
>   	case nir_op_fne:
>   		result = emit_float_cmp(&ctx->ac, LLVMRealUNE, src[0], src[1]);
>   		break;
>   	case nir_op_flt:
> -		result = emit_float_cmp(&ctx->ac, LLVMRealULT, src[0], src[1]);
> +		result = emit_float_cmp(&ctx->ac, LLVMRealOLT, src[0], src[1]);
>   		break;
>   	case nir_op_fge:
> -		result = emit_float_cmp(&ctx->ac, LLVMRealUGE, src[0], src[1]);
> +		result = emit_float_cmp(&ctx->ac, LLVMRealOGE, src[0], src[1]);
>   		break;
>   	case nir_op_fabs:
>   		result = emit_intrin_1f_param(&ctx->ac, "llvm.fabs",
>
On Feb 24, 2018 5:52 AM, "Timothy Arceri" <tarceri@itsqueeze.com> wrote:

Using this on its own I believe will cause CTS regressions, which is what
the other patches were about. Feel free to take on the feedback and come up
with a proper solution. I'm not really sure how to progress this.


The patch is correct. If there are any regression, it's because NIR or GLSL
use incorrect transformations.

Marek




On 24/02/18 00:21, Samuel Pitoiset wrote:

> Original patch from Timothy Arceri, I have just fixed the
> not equal case locally.
>
> This fixes one important rendering issue in Wolfenstein 2
> (the cutscene transition issue).
>
> RadeonSI uses the same ordered comparisons, so I guess that
> what we should do as well.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104302
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104905
> Cc: <mesa-stable@lists.freedesktop.org>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>   src/amd/common/ac_nir_to_llvm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/amd/common/ac_nir_to_llvm.c
> b/src/amd/common/ac_nir_to_llvm.c
> index cccc687157..bc1d16d2a4 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -1801,16 +1801,16 @@ static void visit_alu(struct ac_nir_context *ctx,
> const nir_alu_instr *instr)
>                 result = emit_int_cmp(&ctx->ac, LLVMIntUGE, src[0],
> src[1]);
>                 break;
>         case nir_op_feq:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealUEQ, src[0],
> src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOEQ, src[0],
> src[1]);
>                 break;
>         case nir_op_fne:
>                 result = emit_float_cmp(&ctx->ac, LLVMRealUNE, src[0],
> src[1]);
>                 break;
>         case nir_op_flt:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealULT, src[0],
> src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOLT, src[0],
> src[1]);
>                 break;
>         case nir_op_fge:
> -               result = emit_float_cmp(&ctx->ac, LLVMRealUGE, src[0],
> src[1]);
> +               result = emit_float_cmp(&ctx->ac, LLVMRealOGE, src[0],
> src[1]);
>                 break;
>         case nir_op_fabs:
>                 result = emit_intrin_1f_param(&ctx->ac, "llvm.fabs",
>
> _______________________________________________
mesa-stable mailing list
mesa-stable@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-stable
On 02/24/2018 02:43 PM, Marek Olšák wrote:
> 
> 
> On Feb 24, 2018 5:52 AM, "Timothy Arceri" <tarceri@itsqueeze.com 
> <mailto:tarceri@itsqueeze.com>> wrote:
> 
>     Using this on its own I believe will cause CTS regressions, which is
>     what the other patches were about. Feel free to take on the feedback
>     and come up with a proper solution. I'm not really sure how to
>     progress this.
> 
> 
> The patch is correct. If there are any regression, it's because NIR or 
> GLSL use incorrect transformations.

Yes, I think so.

> 
> Marek
> 
> 
> 
> 
>     On 24/02/18 00:21, Samuel Pitoiset wrote:
> 
>         Original patch from Timothy Arceri, I have just fixed the
>         not equal case locally.
> 
>         This fixes one important rendering issue in Wolfenstein 2
>         (the cutscene transition issue).
> 
>         RadeonSI uses the same ordered comparisons, so I guess that
>         what we should do as well.
> 
>         Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104302
>         <https://bugs.freedesktop.org/show_bug.cgi?id=104302>
>         Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104905
>         <https://bugs.freedesktop.org/show_bug.cgi?id=104905>
>         Cc: <mesa-stable@lists.freedesktop.org
>         <mailto:mesa-stable@lists.freedesktop.org>>
>         Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com
>         <mailto:samuel.pitoiset@gmail.com>>
>         ---
>            src/amd/common/ac_nir_to_llvm.c | 6 +++---
>            1 file changed, 3 insertions(+), 3 deletions(-)
> 
>         diff --git a/src/amd/common/ac_nir_to_llvm.c
>         b/src/amd/common/ac_nir_to_llvm.c
>         index cccc687157..bc1d16d2a4 100644
>         --- a/src/amd/common/ac_nir_to_llvm.c
>         +++ b/src/amd/common/ac_nir_to_llvm.c
>         @@ -1801,16 +1801,16 @@ static void visit_alu(struct
>         ac_nir_context *ctx, const nir_alu_instr *instr)
>                          result = emit_int_cmp(&ctx->ac, LLVMIntUGE,
>         src[0], src[1]);
>                          break;
>                  case nir_op_feq:
>         -               result = emit_float_cmp(&ctx->ac, LLVMRealUEQ,
>         src[0], src[1]);
>         +               result = emit_float_cmp(&ctx->ac, LLVMRealOEQ,
>         src[0], src[1]);
>                          break;
>                  case nir_op_fne:
>                          result = emit_float_cmp(&ctx->ac, LLVMRealUNE,
>         src[0], src[1]);
>                          break;
>                  case nir_op_flt:
>         -               result = emit_float_cmp(&ctx->ac, LLVMRealULT,
>         src[0], src[1]);
>         +               result = emit_float_cmp(&ctx->ac, LLVMRealOLT,
>         src[0], src[1]);
>                          break;
>                  case nir_op_fge:
>         -               result = emit_float_cmp(&ctx->ac, LLVMRealUGE,
>         src[0], src[1]);
>         +               result = emit_float_cmp(&ctx->ac, LLVMRealOGE,
>         src[0], src[1]);
>                          break;
>                  case nir_op_fabs:
>                          result = emit_intrin_1f_param(&ctx->ac,
>         "llvm.fabs",
> 
>     _______________________________________________
>     mesa-stable mailing list
>     mesa-stable@lists.freedesktop.org
>     <mailto:mesa-stable@lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-stable
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-stable>
> 
>