[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 <sroland at vmware.com> wrote:
>> Am 11.10.2015 um 03:29 schrieb Marek Olšák:
>>> From: Marek Olšák <marek.olsak at 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