gm107/ir: Add stg, ldg instructions and function for checking offset length

Submitted by Mark Menzynski on July 19, 2019, 3:33 p.m.

Details

Message ID CA+i2r=vqN7JsNeVq+YxiZhU4bkWp2uRMSThpTd3SR7mZEgmBow@mail.gmail.com
State New
Headers show
Series "gm107/ir: Add stg, ldg instructions and function for checking offset length" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Mark Menzynski July 19, 2019, 3:33 p.m.
On Fri, Jul 19, 2019 at 5:04 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> On Fri, Jul 19, 2019 at 10:57 AM Mark Menzynski <mmenzyns@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@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:

> > +
> > +   int32_t offset = ref.get()->reg.data.offset;
> > +   return offset >  0x7fffff || offset < -0x800000;
>
> You have two spaces after the >. Remove one of them.
>
> > +}
> > +
> >  void
> >  CodeEmitterGM107::emitIMMD(int pos, int len, const ValueRef &ref)
> >  {
> > @@ -2414,6 +2428,17 @@ CodeEmitterGM107::emitLDS()
> >     emitGPR  (0x00, insn->def(0));
> >  }
> >
> > +void
> > +CodeEmitterGM107::emitLDG()
> > +{
> > +   emitInsn (0xeed00000);
> > +   emitLDSTs(0x30, insn->dType);
> > +   emitLDSTc(0x2e);
> > +   emitField(0x2d, 1, insn->src(0).getIndirect(0)->getSize() == 8);
>
> I didn't look, but we don't do something a bit more subtle on the
> other ones, like checking if there's an indirect access in the first
> place? With g[], it almost exclusively will be, but still...
>
> > +   emitADDR (0x08, 0x14, 24, 0, insn->src(0));
> > +   emitGPR  (0x00, insn->def(0));
> > +}
> > +
> >  void
> >  CodeEmitterGM107::emitLD()
> >  {
> > @@ -2445,6 +2470,17 @@ CodeEmitterGM107::emitSTS()
> >     emitGPR  (0x00, insn->src(1));
> >  }
> >
> > +void
> > +CodeEmitterGM107::emitSTG()
> > +{
> > +   emitInsn (0xeed80000);
> > +   emitLDSTs(0x30, insn->dType);
> > +   emitLDSTc(0x2e);
> > +   emitField(0x2d, 1, insn->src(0).getIndirect(0)->getSize() == 8);
> > +   emitADDR (0x08, 0x14, 24, 0, insn->src(0));
> > +   emitGPR  (0x00, insn->src(1));
> > +}
> > +
> >  void
> >  CodeEmitterGM107::emitST()
> >  {
> > --
> > 2.21.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Patch hide | download patch | download mbox

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;
       default:
          assert(!"invalid load");
          emitNOP();

Comments

On Fri, Jul 19, 2019 at 11:34 AM Mark Menzynski <mmenzyns@redhat.com> wrote:
>
> On Fri, Jul 19, 2019 at 5:04 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> >
> > On Fri, Jul 19, 2019 at 10:57 AM Mark Menzynski <mmenzyns@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@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