Re: [Mesa-dev] [PATCH 01/10] gallivm: supply correct opcode info to emit functions

[Marek Olšák]

On Sun, Oct 11, 2015 at 4:22 AM, Roland Scheidegger  wrote:
> Am 11.10.2015 um 03:29 schrieb Marek Olšák:
>> From: Marek Olšák 
>>
>> This is useful only when emit functions use it.
>> The new radeonsi min/max opcode implementation requires this.
>> ---
>>  src/gallium/auxiliary/gallivm/lp_bld_tgsi.c | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c 
>> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
>> index c4ae304..c50d83e 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
>> @@ -114,12 +114,17 @@ lp_build_emit_llvm(
>> struct lp_build_emit_data * emit_data)
>>  {
>> struct lp_build_tgsi_action * action = 
>> _base->op_actions[tgsi_opcode];
>> +   const struct tgsi_opcode_info *old_info = emit_data->info;
>> /* XXX: Assert that this is a componentwise or replicate instruction */
>>
>> lp_build_action_set_dst_type(emit_data, bld_base, tgsi_opcode);
>> emit_data->chan = 0;
>> +
>> +   /* Set and restore the opcode info. */
>> +   emit_data->info = tgsi_get_opcode_info(tgsi_opcode);
>> assert(action->emit);
>> action->emit(action, bld_base, emit_data);
>> +   emit_data->info = old_info;
>> return emit_data->output[0];
>>  }
>>
>>
>
> Could you elaborate why this is necessary? Looks like a hack and I can't
> see why opcode info would be wrong in the first place. Or if that's
> never set correctly and you just need it to be able to distinguish
> min/max later, I'd suggest you shouldn't do that and just use different
> functions.

lp_build_emit_llvm_binary and friends don't set the opcode info at all
and they also don't set the instruction info. This means the emit
function has no way to tell which opcode is being processed if that
emit function is used for multiple opcodes.

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

Re: [Mesa-dev] [PATCH 01/10] gallivm: supply correct opcode info to emit functions

[Marek Olšák]

On Sun, Oct 11, 2015 at 11:15 PM, Roland Scheidegger  wrote:
> So why do you need to set the info back after action->emit? If you want
> to set that always so that information can be used, looks fine to me.
> But if you have to set it back afterwards that screams hack (and I see
> no reason for such a hack as you could easily avoid it by just using
> different emit actions).

A reworked patch is attached, fixing the callers instead of the callee.

Marek
From fe013a4b7a3589ec0b2e75bb49f6e04fb817057a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= 
Date: Sat, 10 Oct 2015 21:24:28 +0200
Subject: [PATCH] gallivm: set correct opcode info from unary/binary/ternary
 emits

and clear the emit_data structure.

The new radeonsi min/max opcode implementation requires this.
---
 src/gallium/auxiliary/gallivm/lp_bld_tgsi.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
index c4ae304..c88dfbf 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
+++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
@@ -129,7 +129,8 @@ lp_build_emit_llvm_unary(
unsigned tgsi_opcode,
LLVMValueRef arg0)
 {
-   struct lp_build_emit_data emit_data;
+   struct lp_build_emit_data emit_data = {{0}};
+   emit_data.info = tgsi_get_opcode_info(tgsi_opcode);
emit_data.arg_count = 1;
emit_data.args[0] = arg0;
return lp_build_emit_llvm(bld_base, tgsi_opcode, _data);
@@ -142,7 +143,8 @@ lp_build_emit_llvm_binary(
LLVMValueRef arg0,
LLVMValueRef arg1)
 {
-   struct lp_build_emit_data emit_data;
+   struct lp_build_emit_data emit_data = {{0}};
+   emit_data.info = tgsi_get_opcode_info(tgsi_opcode);
emit_data.arg_count = 2;
emit_data.args[0] = arg0;
emit_data.args[1] = arg1;
@@ -157,7 +159,8 @@ lp_build_emit_llvm_ternary(
LLVMValueRef arg1,
LLVMValueRef arg2)
 {
-   struct lp_build_emit_data emit_data;
+   struct lp_build_emit_data emit_data = {{0}};
+   emit_data.info = tgsi_get_opcode_info(tgsi_opcode);
emit_data.arg_count = 3;
emit_data.args[0] = arg0;
emit_data.args[1] = arg1;
-- 
2.1.4

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

Re: [Mesa-dev] [PATCH 01/10] gallivm: supply correct opcode info to emit functions

[Roland Scheidegger]

Am 12.10.2015 um 00:55 schrieb Marek Olšák:
> On Sun, Oct 11, 2015 at 11:15 PM, Roland Scheidegger  
> wrote:
>> So why do you need to set the info back after action->emit? If you want
>> to set that always so that information can be used, looks fine to me.
>> But if you have to set it back afterwards that screams hack (and I see
>> no reason for such a hack as you could easily avoid it by just using
>> different emit actions).
> 
> A reworked patch is attached, fixing the callers instead of the callee.
> 
> Marek
> 

Thanks, that looks much more reasonable now.

Roland

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

Re: [Mesa-dev] [PATCH 01/10] gallivm: supply correct opcode info to emit functions

[Roland Scheidegger]

Am 11.10.2015 um 12:39 schrieb Marek Olšák:
> On Sun, Oct 11, 2015 at 4:22 AM, Roland Scheidegger  
> wrote:
>> Am 11.10.2015 um 03:29 schrieb Marek Olšák:
>>> From: Marek Olšák 
>>>
>>> This is useful only when emit functions use it.
>>> The new radeonsi min/max opcode implementation requires this.
>>> ---
>>>  src/gallium/auxiliary/gallivm/lp_bld_tgsi.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c 
>>> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
>>> index c4ae304..c50d83e 100644
>>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
>>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
>>> @@ -114,12 +114,17 @@ lp_build_emit_llvm(
>>> struct lp_build_emit_data * emit_data)
>>>  {
>>> struct lp_build_tgsi_action * action = 
>>> _base->op_actions[tgsi_opcode];
>>> +   const struct tgsi_opcode_info *old_info = emit_data->info;
>>> /* XXX: Assert that this is a componentwise or replicate instruction */
>>>
>>> lp_build_action_set_dst_type(emit_data, bld_base, tgsi_opcode);
>>> emit_data->chan = 0;
>>> +
>>> +   /* Set and restore the opcode info. */
>>> +   emit_data->info = tgsi_get_opcode_info(tgsi_opcode);
>>> assert(action->emit);
>>> action->emit(action, bld_base, emit_data);
>>> +   emit_data->info = old_info;
>>> return emit_data->output[0];
>>>  }
>>>
>>>
>>
>> Could you elaborate why this is necessary? Looks like a hack and I can't
>> see why opcode info would be wrong in the first place. Or if that's
>> never set correctly and you just need it to be able to distinguish
>> min/max later, I'd suggest you shouldn't do that and just use different
>> functions.
> 
> lp_build_emit_llvm_binary and friends don't set the opcode info at all
> and they also don't set the instruction info. This means the emit
> function has no way to tell which opcode is being processed if that
> emit function is used for multiple opcodes.
> 
> Marek
> 

So why do you need to set the info back after action->emit? If you want
to set that always so that information can be used, looks fine to me.
But if you have to set it back afterwards that screams hack (and I see
no reason for such a hack as you could easily avoid it by just using
different emit actions).

Roland

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

[Mesa-dev] [PATCH 01/10] gallivm: supply correct opcode info to emit functions

[Marek Olšák]

From: Marek Olšák 

This is useful only when emit functions use it.
The new radeonsi min/max opcode implementation requires this.
---
 src/gallium/auxiliary/gallivm/lp_bld_tgsi.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c 
b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
index c4ae304..c50d83e 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
+++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
@@ -114,12 +114,17 @@ lp_build_emit_llvm(
struct lp_build_emit_data * emit_data)
 {
struct lp_build_tgsi_action * action = _base->op_actions[tgsi_opcode];
+   const struct tgsi_opcode_info *old_info = emit_data->info;
/* XXX: Assert that this is a componentwise or replicate instruction */
 
lp_build_action_set_dst_type(emit_data, bld_base, tgsi_opcode);
emit_data->chan = 0;
+
+   /* Set and restore the opcode info. */
+   emit_data->info = tgsi_get_opcode_info(tgsi_opcode);
assert(action->emit);
action->emit(action, bld_base, emit_data);
+   emit_data->info = old_info;
return emit_data->output[0];
 }
 
-- 
2.1.4

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

Re: [Mesa-dev] [PATCH 01/10] gallivm: supply correct opcode info to emit functions

[Roland Scheidegger]

Am 11.10.2015 um 03:29 schrieb Marek Olšák:
> From: Marek Olšák 
> 
> This is useful only when emit functions use it.
> The new radeonsi min/max opcode implementation requires this.
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_tgsi.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c 
> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
> index c4ae304..c50d83e 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi.c
> @@ -114,12 +114,17 @@ lp_build_emit_llvm(
> struct lp_build_emit_data * emit_data)
>  {
> struct lp_build_tgsi_action * action = _base->op_actions[tgsi_opcode];
> +   const struct tgsi_opcode_info *old_info = emit_data->info;
> /* XXX: Assert that this is a componentwise or replicate instruction */
>  
> lp_build_action_set_dst_type(emit_data, bld_base, tgsi_opcode);
> emit_data->chan = 0;
> +
> +   /* Set and restore the opcode info. */
> +   emit_data->info = tgsi_get_opcode_info(tgsi_opcode);
> assert(action->emit);
> action->emit(action, bld_base, emit_data);
> +   emit_data->info = old_info;
> return emit_data->output[0];
>  }
>  
> 

Could you elaborate why this is necessary? Looks like a hack and I can't
see why opcode info would be wrong in the first place. Or if that's
never set correctly and you just need it to be able to distinguish
min/max later, I'd suggest you shouldn't do that and just use different
functions.

Roland

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