[Mesa-dev] [PATCH 04/15] i965/fs: Get rid of reladdr

[Jason Ekstrand]

On Dec 10, 2015 6:58 AM, "Francisco Jerez" <currojerez at riseup.net> wrote:
>
> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> > We aren't using it anymore.
>
> It seems useful to me to be able to represent indirect access as part of
> any instruction source or destination register.
>
> The following:
>
> | mov_indirect g0, g1, a0
> | foo g2, g0
>
> and the converse case with indirect destination offset (which you don't
> seem to represent currently) can be implemented by the hardware more
> efficiently using a single instruction in certain cases.  The current IR
> is able to represent what the hardware can do, but supporting the
> MOV_INDIRECT instruction only would force us to keep the indirection
> separate from the instruction that uses it, so it seems like a less
> expressive representation to me than the current approach, unless you're
> willing to add _INDIRECT variants of most hardware opcodes.

Yes and, mostly, no.  Yes, you can put an indirect on almost anything but
it has substantial restrictions:

1) Destination indirects must be uniform (I'm only 95% sure this is the
case)

2) We only have 8 address subregisters on HSW and prior and 16 on BDW which
means:

    a) All indirects must be uniform OR

    b) We must be on Broadwell, have only two indirects, and split the
instruction OR

    c) We can only have one indirect (and still split the instruction on
HSW)

So, yes, "everthing can be uniform", but not in real life.  The reladdr
abstraction, while maybe ok for a high-level IR, doesn't accurately
represent the hardware at all because it can't represent any of the
restrictions without piles of helper functions and checks.  The
MOV_INDIRECT instruction, on the other hand, is something we can always do
because it falls in the "one indirect" case above.  The reladdr abstraction
also suffers from the recursive reladdr problem which MOV_INDIRECT neatly
side-steps by design.  We can't even use reladdr past initial NIR -> FS
stage because none of the optimization passes know what to do with it (and
no, I don't want to add reladdr support to any of them).

Yes, removing reladdr makes the IR "less expressive", but we're not really
using that expressiveness nor is reladdr the abstraction we want.  If we
want to start taking more advantage of relative addressing in the future,
we can come up with a better abstraction then.

--Jason

> > ---
> >  src/mesa/drivers/dri/i965/brw_fs.cpp  | 7 +------
> >  src/mesa/drivers/dri/i965/brw_ir_fs.h | 5 +----
> >  2 files changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > index 7cc03c5..786c5fb 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > @@ -433,7 +433,6 @@ fs_reg::fs_reg(struct ::brw_reg reg) :
> >  {
> >     this->reg_offset = 0;
> >     this->subreg_offset = 0;
> > -   this->reladdr = NULL;
> >     this->stride = 1;
> >     if (this->file == IMM &&
> >         (this->type != BRW_REGISTER_TYPE_V &&
> > @@ -448,7 +447,6 @@ fs_reg::equals(const fs_reg &r) const
> >  {
> >     return (this->backend_reg::equals(r) &&
> >             subreg_offset == r.subreg_offset &&
> > -           !reladdr && !r.reladdr &&
> >             stride == r.stride);
> >  }
> >
> > @@ -4716,9 +4714,7 @@ fs_visitor::dump_instruction(backend_instruction
*be_inst, FILE *file)
> >           break;
> >        case UNIFORM:
> >           fprintf(file, "u%d", inst->src[i].nr +
inst->src[i].reg_offset);
> > -         if (inst->src[i].reladdr) {
> > -            fprintf(file, "+reladdr");
> > -         } else if (inst->src[i].subreg_offset) {
> > +         if (inst->src[i].subreg_offset) {
> >              fprintf(file, "+%d.%d", inst->src[i].reg_offset,
> >                      inst->src[i].subreg_offset);
> >           }
> > @@ -4829,7 +4825,6 @@
fs_visitor::get_instruction_generating_reg(fs_inst *start,
> >  {
> >     if (end == start ||
> >         end->is_partial_write() ||
> > -       reg.reladdr ||
> >         !reg.equals(end->dst)) {
> >        return NULL;
> >     } else {
> > diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h
b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > index c3eec2e..e4f20f4 100644
> > --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > @@ -58,8 +58,6 @@ public:
> >      */
> >     int subreg_offset;
> >
> > -   fs_reg *reladdr;
> > -
> >     /** Register region horizontal stride */
> >     uint8_t stride;
> >  };
> > @@ -136,8 +134,7 @@ component(fs_reg reg, unsigned idx)
> >  static inline bool
> >  is_uniform(const fs_reg &reg)
> >  {
> > -   return (reg.stride == 0 || reg.is_null()) &&
> > -          (!reg.reladdr || is_uniform(*reg.reladdr));
> > +   return (reg.stride == 0 || reg.is_null());
> >  }
> >
> >  /**
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151210/6c7f3449/attachment.html>