[Mesa-dev] [PATCH 13/78] i965/nir/vec4: Implement conditional statements (nir_cf_node_if)

[Jason Ekstrand]

On Fri, Jul 3, 2015 at 12:32 AM, Iago Toral <itoral at igalia.com> wrote:
> On Thu, 2015-07-02 at 10:11 -0700, Jason Ekstrand wrote:
>> On Wed, Jul 1, 2015 at 11:44 PM, Iago Toral <itoral at igalia.com> wrote:
>> > On Tue, 2015-06-30 at 09:30 -0700, Jason Ekstrand wrote:
>> >> On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
>> >> > From: Iago Toral Quiroga <itoral at igalia.com>
>> >> >
>> >> > The same we do in the FS NIR backend, only that here we need to consider
>> >> > the number of components in the condition and adjust the swizzle
>> >> > accordingly.
>> >> >
>> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580
>> >> > ---
>> >> >  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 23 ++++++++++++++++++++++-
>> >> >  1 file changed, 22 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> >> > index 1ec75ee..d81b6a7 100644
>> >> > --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> >> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
>> >> > @@ -314,7 +314,28 @@ vec4_visitor::nir_emit_cf_list(exec_list *list)
>> >> >  void
>> >> >  vec4_visitor::nir_emit_if(nir_if *if_stmt)
>> >> >  {
>> >> > -   /* @TODO: Not yet implemented */
>> >> > +   /* First, put the condition in f0 */
>> >> > +   src_reg condition = get_nir_src(if_stmt->condition, BRW_REGISTER_TYPE_D);
>> >> > +
>> >> > +   int num_components = if_stmt->condition.is_ssa ?
>> >> > +      if_stmt->condition.ssa->num_components :
>> >> > +      if_stmt->condition.reg.reg->num_components;
>> >> > +
>> >> > +   condition.swizzle = brw_swizzle_for_size(num_components);
>> >> > +
>> >> > +   vec4_instruction *inst = emit(MOV(dst_null_d(), condition));
>> >> > +   inst->conditional_mod = BRW_CONDITIONAL_NZ;
>> >>
>> >> NIR if statements read only one component by definition.  There's no
>> >> need to do this.
>> >
>> > I see, we still need to do this explicitly though:
>> >
>> > condition.swizzle = brw_swizzle_for_size(1);
>> >
>> > Maybe we should just make get_nir_src() set the swizzle based on the
>> > number of components instead so we don't have to do this kind of things
>> > after calling that, does that sound better?
>>
>> Just pass the number of components into get_nir_src()?  That sounds fine to me.
>> --Jason
>
> Is that better? In this case it is sufficient because it is always 1,
> but in other cases where we call get_nir_src I imagine we would also
> want to get a register with the swizzle preset based on the number of
> components the nir src, right? In that case, the number of components we
> would need to pass would come from doing something like:
>
> int num_components = src.is_ssa ?
>                         src.ssa->num_components :
>                         src.reg.reg->num_components;

That's not quite what you want either.  NIR doesn't guarantee that the
number of components used equals the number of components in the
register or ssa value.  (We probably want to change that eventually).
However, you should always know how many components you have.  For ALU
operations, it's either 4 (if it has a write-mask) or it's in the op
infos; for intrinsics it's either in the intrinsic_infos or in
intrin->num_components; for if's it's, always 1, etc.

Does that make more sense?  Also, this should affect the e-mail that I
just sent to Antia.

--Jason

> and doing this every time we call get_nir_src does not look very nice to
> me. Since we are already passing the nir src to get_nir_src, wouldn't it
> be better if we had that function do this for us?
>
> Iago
>
>> >>
>> >> > +   emit(IF(BRW_PREDICATE_NORMAL));
>> >> > +
>> >> > +   nir_emit_cf_list(&if_stmt->then_list);
>> >> > +
>> >> > +   /* note: if the else is empty, dead CF elimination will remove it */
>> >> > +   emit(BRW_OPCODE_ELSE);
>> >> > +
>> >> > +   nir_emit_cf_list(&if_stmt->else_list);
>> >> > +
>> >> > +   emit(BRW_OPCODE_ENDIF);
>> >> >  }
>> >> >
>> >> >  void
>> >> > --
>> >> > 2.1.4
>> >> >
>> >> > _______________________________________________
>> >> > mesa-dev mailing list
>> >> > mesa-dev at lists.freedesktop.org
>> >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> >> _______________________________________________
>> >> mesa-dev mailing list
>> >> mesa-dev at lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> >
>> >
>>
>
>