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

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

Details

Message ID 1444526990-14029-2-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>

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(+)

Patch hide | download patch | download mbox

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 = &bld_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];
 }
 

Comments

Am 11.10.2015 um 03:29 schrieb Marek Olšák:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> 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 = &bld_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
On Sun, Oct 11, 2015 at 4:22 AM, Roland Scheidegger <sroland@vmware.com> wrote:
> Am 11.10.2015 um 03:29 schrieb Marek Olšák:
>> From: Marek Olšák <marek.olsak@amd.com>
>>
>> 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 = &bld_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
Am 11.10.2015 um 12:39 schrieb Marek Olšák:
> On Sun, Oct 11, 2015 at 4:22 AM, Roland Scheidegger <sroland@vmware.com> wrote:
>> Am 11.10.2015 um 03:29 schrieb Marek Olšák:
>>> From: Marek Olšák <marek.olsak@amd.com>
>>>
>>> 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 = &bld_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