[Mesa-dev,02/10] gallivm: implement the correct version of LRP

Submitted by Marek Olšák on Oct. 11, 2015, 1:29 a.m.

Details

Message ID 1444526990-14029-3-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák Oct. 11, 2015, 1:29 a.m.
From: Marek Olšák <marek.olsak@amd.com>

The previous version has precision issues. This can be a problem
with tessellation. Sadly, I can't find the article where I read it
anymore. I'm not sure if the unsafe-fp-math flag would be enough to revert
this.
---
 src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
index 0ad78b0..512558b 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
+++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
@@ -538,12 +538,13 @@  lrp_emit(
    struct lp_build_tgsi_context * bld_base,
    struct lp_build_emit_data * emit_data)
 {
-   LLVMValueRef tmp;
-   tmp = lp_build_emit_llvm_binary(bld_base, TGSI_OPCODE_SUB,
-                                   emit_data->args[1],
-                                   emit_data->args[2]);
-   emit_data->output[emit_data->chan] = lp_build_emit_llvm_ternary(bld_base,
-                    TGSI_OPCODE_MAD, emit_data->args[0], tmp, emit_data->args[2]);
+   struct lp_build_context *bld = &bld_base->base;
+   LLVMValueRef inv, a, b;
+
+   inv = lp_build_sub(bld, bld_base->base.one, emit_data->args[0]);
+   a = lp_build_mul(bld, emit_data->args[1], emit_data->args[0]);
+   b = lp_build_mul(bld, emit_data->args[2], inv);
+   emit_data->output[emit_data->chan] = lp_build_add(bld, a, b);
 }
 
 /* TGSI_OPCODE_MAD */

Comments

Any comment or is this okay with people? Given, "(1-t)*a + t*b", the
original code didn't return b for t=1 because it's "floating-point".

Marek

On Sun, Oct 11, 2015 at 3:29 AM, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> The previous version has precision issues. This can be a problem
> with tessellation. Sadly, I can't find the article where I read it
> anymore. I'm not sure if the unsafe-fp-math flag would be enough to revert
> this.
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
> index 0ad78b0..512558b 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
> @@ -538,12 +538,13 @@ lrp_emit(
>     struct lp_build_tgsi_context * bld_base,
>     struct lp_build_emit_data * emit_data)
>  {
> -   LLVMValueRef tmp;
> -   tmp = lp_build_emit_llvm_binary(bld_base, TGSI_OPCODE_SUB,
> -                                   emit_data->args[1],
> -                                   emit_data->args[2]);
> -   emit_data->output[emit_data->chan] = lp_build_emit_llvm_ternary(bld_base,
> -                    TGSI_OPCODE_MAD, emit_data->args[0], tmp, emit_data->args[2]);
> +   struct lp_build_context *bld = &bld_base->base;
> +   LLVMValueRef inv, a, b;
> +
> +   inv = lp_build_sub(bld, bld_base->base.one, emit_data->args[0]);
> +   a = lp_build_mul(bld, emit_data->args[1], emit_data->args[0]);
> +   b = lp_build_mul(bld, emit_data->args[2], inv);
> +   emit_data->output[emit_data->chan] = lp_build_add(bld, a, b);
>  }
>
>  /* TGSI_OPCODE_MAD */
> --
> 2.1.4
>
Am 15.10.2015 um 16:44 schrieb Marek Olšák:
> Any comment or is this okay with people? Given, "(1-t)*a + t*b", the
> original code didn't return b for t=1 because it's "floating-point".
> 
> Marek
> 
> On Sun, Oct 11, 2015 at 3:29 AM, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> The previous version has precision issues. This can be a problem
>> with tessellation. Sadly, I can't find the article where I read it
>> anymore. I'm not sure if the unsafe-fp-math flag would be enough to revert
>> this.
>> ---
>>  src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>> index 0ad78b0..512558b 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>> @@ -538,12 +538,13 @@ lrp_emit(
>>     struct lp_build_tgsi_context * bld_base,
>>     struct lp_build_emit_data * emit_data)
>>  {
>> -   LLVMValueRef tmp;
>> -   tmp = lp_build_emit_llvm_binary(bld_base, TGSI_OPCODE_SUB,
>> -                                   emit_data->args[1],
>> -                                   emit_data->args[2]);
>> -   emit_data->output[emit_data->chan] = lp_build_emit_llvm_ternary(bld_base,
>> -                    TGSI_OPCODE_MAD, emit_data->args[0], tmp, emit_data->args[2]);
>> +   struct lp_build_context *bld = &bld_base->base;
>> +   LLVMValueRef inv, a, b;
>> +
>> +   inv = lp_build_sub(bld, bld_base->base.one, emit_data->args[0]);
>> +   a = lp_build_mul(bld, emit_data->args[1], emit_data->args[0]);
>> +   b = lp_build_mul(bld, emit_data->args[2], inv);
>> +   emit_data->output[emit_data->chan] = lp_build_add(bld, a, b);
>>  }
>>
>>  /* TGSI_OPCODE_MAD */
>> --

Please add a comment why it's using t*a + (1-t)*b and not (a-b)*t + b.
Though it is yet another thing we should have some more control over in
tgsi. Because if you're willing to allow unsafe-fp-math, then you should
also be willing to accept the simpler formula (I'm quite sure
unsafe-fp-math would be allowed to turn one formula into the other).
But otherwise I guess this is ok - it is the formula specified by glsl
after all.

Roland
Roland was on PTO.

IMO, the change makes sense from a numeric accuracy POV.

I fear this might cause some slowdown with llvmpipe (two muls intead of 
one), but hopefully it won't be significant.  The accuracy issue could 
cause glitches to llvmpipe too.

Jose

On 15/10/15 15:44, Marek Olšák wrote:
> Any comment or is this okay with people? Given, "(1-t)*a + t*b", the
> original code didn't return b for t=1 because it's "floating-point".
>
> Marek
>
> On Sun, Oct 11, 2015 at 3:29 AM, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> The previous version has precision issues. This can be a problem
>> with tessellation. Sadly, I can't find the article where I read it
>> anymore. I'm not sure if the unsafe-fp-math flag would be enough to revert
>> this.
>> ---
>>   src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>> index 0ad78b0..512558b 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>> @@ -538,12 +538,13 @@ lrp_emit(
>>      struct lp_build_tgsi_context * bld_base,
>>      struct lp_build_emit_data * emit_data)
>>   {
>> -   LLVMValueRef tmp;
>> -   tmp = lp_build_emit_llvm_binary(bld_base, TGSI_OPCODE_SUB,
>> -                                   emit_data->args[1],
>> -                                   emit_data->args[2]);
>> -   emit_data->output[emit_data->chan] = lp_build_emit_llvm_ternary(bld_base,
>> -                    TGSI_OPCODE_MAD, emit_data->args[0], tmp, emit_data->args[2]);
>> +   struct lp_build_context *bld = &bld_base->base;
>> +   LLVMValueRef inv, a, b;
>> +
>> +   inv = lp_build_sub(bld, bld_base->base.one, emit_data->args[0]);
>> +   a = lp_build_mul(bld, emit_data->args[1], emit_data->args[0]);
>> +   b = lp_build_mul(bld, emit_data->args[2], inv);
>> +   emit_data->output[emit_data->chan] = lp_build_add(bld, a, b);
>>   }
>>
>>   /* TGSI_OPCODE_MAD */
>> --
>> 2.1.4
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On 15/10/15 16:20, Roland Scheidegger wrote:
> Am 15.10.2015 um 16:44 schrieb Marek Olšák:
>> Any comment or is this okay with people? Given, "(1-t)*a + t*b", the
>> original code didn't return b for t=1 because it's "floating-point".
>>
>> Marek
>>
>> On Sun, Oct 11, 2015 at 3:29 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> The previous version has precision issues. This can be a problem
>>> with tessellation. Sadly, I can't find the article where I read it
>>> anymore. I'm not sure if the unsafe-fp-math flag would be enough to revert
>>> this.
>>> ---
>>>   src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> index 0ad78b0..512558b 100644
>>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> @@ -538,12 +538,13 @@ lrp_emit(
>>>      struct lp_build_tgsi_context * bld_base,
>>>      struct lp_build_emit_data * emit_data)
>>>   {
>>> -   LLVMValueRef tmp;
>>> -   tmp = lp_build_emit_llvm_binary(bld_base, TGSI_OPCODE_SUB,
>>> -                                   emit_data->args[1],
>>> -                                   emit_data->args[2]);
>>> -   emit_data->output[emit_data->chan] = lp_build_emit_llvm_ternary(bld_base,
>>> -                    TGSI_OPCODE_MAD, emit_data->args[0], tmp, emit_data->args[2]);
>>> +   struct lp_build_context *bld = &bld_base->base;
>>> +   LLVMValueRef inv, a, b;
>>> +
>>> +   inv = lp_build_sub(bld, bld_base->base.one, emit_data->args[0]);
>>> +   a = lp_build_mul(bld, emit_data->args[1], emit_data->args[0]);
>>> +   b = lp_build_mul(bld, emit_data->args[2], inv);
>>> +   emit_data->output[emit_data->chan] = lp_build_add(bld, a, b);
>>>   }
>>>
>>>   /* TGSI_OPCODE_MAD */
>>> --
>
> Please add a comment why it's using t*a + (1-t)*b and not (a-b)*t + b.
> Though it is yet another thing we should have some more control over in
> tgsi.

 > Because if you're willing to allow unsafe-fp-math, then you should
> also be willing to accept the simpler formula (I'm quite sure
> unsafe-fp-math would be allowed to turn one formula into the other).

Yep, that's my understanding of "unsafe fp math" too.

Jose