[Mesa-dev] [PATCH] gm107/ir: Add stg, ldg instructions and function for checking offset length

[Ilia Mirkin]

On Fri, Jul 19, 2019 at 11:34 AM Mark Menzynski <mmenzyns at redhat.com> wrote:
>
> On Fri, Jul 19, 2019 at 5:04 PM Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> >
> > On Fri, Jul 19, 2019 at 10:57 AM Mark Menzynski <mmenzyns at redhat.com> wrote:
> > >
> > > Nvidia actively uses these instructions, maybe they are better in
> > > something.
> > > Long offset checking function was made because these functions only have 24 bit
> > > address offsets.
> > >
> > > Signed-off-by: Mark Menzynski <mmenzyns at redhat.com>
> > > ---
> > >  .../nouveau/codegen/nv50_ir_emit_gm107.cpp    | 36 +++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> > > diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> > > index 6eefe8f0025..c01a3017ba9 100644
> > > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> > > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> > > @@ -87,6 +87,7 @@ private:
> > >     inline void emitADDR(int, int, int, int, const ValueRef &);
> > >     inline void emitCBUF(int, int, int, int, int, const ValueRef &);
> > >     inline bool longIMMD(const ValueRef &);
> > > +   inline bool longOffset(const ValueRef &);
> > >     inline void emitIMMD(int, int, const ValueRef &);
> > >
> > >     void emitCond3(int, CondCode);
> > > @@ -174,9 +175,11 @@ private:
> > >     void emitLDC();
> > >     void emitLDL();
> > >     void emitLDS();
> > > +   void emitLDG();
> > >     void emitLD();
> > >     void emitSTL();
> > >     void emitSTS();
> > > +   void emitSTG();
> > >     void emitST();
> > >     void emitALD();
> > >     void emitAST();
> > > @@ -333,6 +336,17 @@ CodeEmitterGM107::longIMMD(const ValueRef &ref)
> > >     return false;
> > >  }
> > >
> > > +bool
> > > +CodeEmitterGM107::longOffset(const ValueRef &ref)
> > > +{
> > > +   // TODO: check for other files as well?
> > > +   if (ref.getFile() != FILE_MEMORY_GLOBAL)
> > > +      return false;
> >
> > I haven't seen the uses (best to send stuff like this as a series),
> > but you're saying that if it's not global memory, then it's not a long
> > offset? I suspect in the caller it should be more like
> >
> > assert(file == global || !long offset) or something.
> >
> This is how I used it for Load. Store was used the same way. I have
> not sent it because we didn't found any noticeable changes with that:
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> index c01a3017ba9..f632178138b 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
> @@ -3603,7 +3603,12 @@ CodeEmitterGM107::emitInstruction(Instruction *i)
>        case FILE_MEMORY_CONST : emitLDC(); break;
>        case FILE_MEMORY_LOCAL : emitLDL(); break;
>        case FILE_MEMORY_SHARED: emitLDS(); break;
> -      case FILE_MEMORY_GLOBAL: emitLD(); break;
> +      case FILE_MEMORY_GLOBAL:
> +         if (longOffset(insn->src(0)))
> +            emitLD();
> +         else
> +            emitLDG();
> +         break;

OK. I'd dump the check from longOffset -- all it says is if an offset
is long. It just seems awkward as-is.

If people feel strongly though, I could be convinced otherwise.

  -ilia