On Thu, Jul 2, 2015 at 2:54 AM, Eduardo Lima Mitev <el...@igalia.com> wrote:
> On 06/30/2015 06:26 PM, Jason Ekstrand wrote:
>> On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <el...@igalia.com> wrote:
>>> From: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
>>>
>>> These methods are essential for the implementation of the NIR->vec4 pass.
>>> They
>>> work similar to their fs_nir counter-parts.
>>>
>>> When processing instructions, these methods are invoked to resolve the
>>> brw registers (source or destination) corresponding to the NIR sources
>>> or destination. It builds a map of NIR register index to brw register for
>>> all registers locally allocated in a block.
>>>
>>> In the case of get_nir_src(), it also builds immediate registers on-the-fly
>>> when queried for a SSA source which at this point can only correspond to
>>> constant values.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580
>>> ---
>>> src/mesa/drivers/dri/i965/brw_vec4.h | 7 ++
>>> src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 102
>>> +++++++++++++++++++++++++++++
>>> 2 files changed, 109 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
>>> b/src/mesa/drivers/dri/i965/brw_vec4.h
>>> index d837d90..a0f5935 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
>>> @@ -411,6 +411,13 @@ public:
>>> virtual void nir_emit_jump(nir_jump_instr *instr);
>>> virtual void nir_emit_texture(nir_tex_instr *instr);
>>>
>>> + dst_reg get_nir_dest(nir_dest dest, enum brw_reg_type type);
>>> + dst_reg get_nir_dest(nir_dest dest, nir_alu_type type);
>>> + dst_reg get_nir_dest(nir_dest dest);
>>> + src_reg get_nir_src(nir_src src, enum brw_reg_type type);
>>> + src_reg get_nir_src(nir_src src, nir_alu_type type);
>>> + src_reg get_nir_src(nir_src src);
>>> +
>>> virtual dst_reg *make_reg_for_system_value(int location,
>>> const glsl_type *type) = 0;
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>> b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>> index 36c9dc0..1ec75ee 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>>> @@ -365,6 +365,108 @@ vec4_visitor::nir_emit_instr(nir_instr *instr)
>>> }
>>> }
>>>
>>> +static dst_reg
>>> +dst_reg_for_nir_reg(vec4_visitor *v, nir_register *nir_reg,
>>> + unsigned base_offset, nir_src *indirect)
>>> +{
>>> + dst_reg reg;
>>> +
>>> + reg = v->nir_locals[nir_reg->index];
>>> +
>>> + reg = offset(reg, base_offset * nir_reg->num_components);
>>> + if (indirect) {
>>> + int multiplier = nir_reg->num_components;
>>> +
>>> + reg.reladdr = new(v->mem_ctx) src_reg(dst_reg(GRF,
>>> v->alloc.allocate(1)));
>>> + v->emit(v->MUL(dst_reg(*reg.reladdr),
>>> + retype(v->get_nir_src(*indirect),
>>> BRW_REGISTER_TYPE_D),
>>> + src_reg(multiplier)));
>>
>> I'm not sure this is correct. We need this in the FS backend because
>> each register is a scalar value. In vec4, each array element should
>> correspond to a single vec4 register so you shouldn't need to
>> multiply. Actually, I think it's currently dead code in the backend
>> because we lower it all away, so it may not be correct at all.
>>
>
> You are right, this multiply is not needed. I already removed it and
> verified it. This will be changed in v2.
>
>>> + }
>>> +
>>> + return reg;
>>> +}
>>> +
>>> +dst_reg
>>> +vec4_visitor::get_nir_dest(nir_dest dest)
>>> +{
>>> + return dst_reg_for_nir_reg(this, dest.reg.reg, dest.reg.base_offset,
>>> + dest.reg.indirect);
>>> +}
>>> +
>>> +dst_reg
>>> +vec4_visitor::get_nir_dest(nir_dest dest, enum brw_reg_type type)
>>> +{
>>> + dst_reg reg = get_nir_dest(dest);
>>> + return retype(reg, type);
>>> +}
>>> +
>>> +dst_reg
>>> +vec4_visitor::get_nir_dest(nir_dest dest, nir_alu_type type)
>>> +{
>>> + dst_reg reg = get_nir_dest(dest);
>>> + return retype(reg, brw_type_for_nir_type(type));
>>> +}
>>> +
>>> +src_reg
>>> +vec4_visitor::get_nir_src(nir_src src, enum brw_reg_type type)
>>> +{
>>> + dst_reg reg;
>>> +
>>> + if (src.is_ssa) {
>>> + assert(src.ssa->parent_instr->type == nir_instr_type_load_const);
>>> + nir_load_const_instr *load =
>>> nir_instr_as_load_const(src.ssa->parent_instr);
>>> +
>>> + reg = dst_reg(GRF, alloc.allocate(src.ssa->num_components));
>>
>> It's vec4, you only need one register, not num_components-many.
>>
>
> Right. This comes from our initial misconception of how the units were
> given in vec4 vs. scalar. I have changed that everywhere in the backend
> already, to allocate exactly 1 unit.
>
>>> + reg = retype(reg, type);
>>> +
>>> + /* @FIXME: while this is what fs_nir does, we can do this better in
>>> the VS
>>> + * stage because we can emit vector operations and save some MOVs in
>>> + * cases where the constants are representable in 8 bits.
>>> + * So by now, we emit a MOV for each component.
>>> + */
>>> + for (unsigned i = 0; i < src.ssa->num_components; ++i) {
>>> + reg.writemask = 1 << i;
>>> +
>>> + switch (reg.type) {
>>> + case BRW_REGISTER_TYPE_F:
>>> + emit(MOV(reg, src_reg(load->value.f[i])));
>>> + break;
>>> + case BRW_REGISTER_TYPE_D:
>>> + emit(MOV(reg, src_reg(load->value.i[i])));
>>> + break;
>>> + case BRW_REGISTER_TYPE_UD:
>>> + emit(MOV(reg, src_reg(load->value.u[i])));
>>> + break;
>>> + default:
>>> + unreachable("invalid register type");
>>> + }
>>> + }
>>> +
>>> + /* Set final writemask */
>>> + reg.writemask = brw_writemask_for_size(src.ssa->num_components);
>>> + }
>>> + else {
>>> + reg = dst_reg_for_nir_reg(this, src.reg.reg, src.reg.base_offset,
>>> + src.reg.indirect);
>>> + reg = retype(reg, type);
>>
>> Why do you support indirects for destinations but not here?
>>
>
> It is indeed supported. Notice I'm calling dst_reg_for_nir_reg(), which
> has the indirect handling. get_nir_dst() also uses it.
Sorry, I missed the chaining there. This is fine.
>>> + }
>>> +
>>> + return src_reg(reg);
>>> +}
>>> +
>>> +src_reg
>>> +vec4_visitor::get_nir_src(nir_src src, nir_alu_type type)
>>> +{
>>> + return get_nir_src(src, brw_type_for_nir_type(type));
>>> +}
>>> +
>>> +src_reg
>>> +vec4_visitor::get_nir_src(nir_src src)
>>> +{
>>> + /* if type is not specified, default to signed int */
>>> + return get_nir_src(src, nir_type_int);
>>> +}
>>> +
>>> void
>>> vec4_visitor::nir_emit_intrinsic(nir_intrinsic_instr *instr)
>>> {
>>> --
>>> 2.1.4
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
