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

[Iago Toral]

On Sun, 2015-07-05 at 19:14 -0700, Jason Ekstrand wrote:
> 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?

Yes, thanks for explaining this. I recall now that Connor had mentioned
something like this once as well.

Iago

> 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
> >> >
> >> >
> >>
> >
> >
>