[Mesa-dev] [PATCH v2 08/13] nvc0/ir: add support for compute UBOs on Kepler

[Samuel Pitoiset]

On 04/01/2016 07:40 AM, Ilia Mirkin wrote:
>
> On Mar 31, 2016 12:09 PM, "Samuel Pitoiset" <samuel.pitoiset at gmail.com
> <mailto:samuel.pitoiset at gmail.com>> wrote:
>  >
>  > Make sure to avoid out of bounds access in presence of indirect
>  > array indexing by loading the size from the driver constant buffer.
>  >
>  > Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com
> <mailto:samuel.pitoiset at gmail.com>>
>  > ---
>  >  .../nouveau/codegen/nv50_ir_lowering_nvc0.cpp      | 55
> +++++++++++++++++++++-
>  >  .../nouveau/codegen/nv50_ir_lowering_nvc0.h        |  3 ++
>  >  2 files changed, 57 insertions(+), 1 deletion(-)
>  >
>  > diff --git
> a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>  > index 850147b..989af20 100644
>  > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>  > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>  > @@ -1322,6 +1322,24 @@ NVC0LoweringPass::loadBufLength32(Value *ptr,
> uint32_t off)
>  >  }
>  >
>  >  inline Value *
>  > +NVC0LoweringPass::loadUboInfo32(Value *ptr, uint32_t off)
>  > +{
>  > +   return loadResInfo32(ptr, off, prog->driver->io.uboInfoBase);
>  > +}
>  > +
>  > +inline Value *
>  > +NVC0LoweringPass::loadUboInfo64(Value *ptr, uint32_t off)
>  > +{
>  > +   return loadResInfo64(ptr, off, prog->driver->io.uboInfoBase);
>
> Something somewhere needs to clamp the offset to the max possible const
> buf... I guess I don't do this for shader buffers either :( put in a
> comment maybe so we don't forget?

Right, this can be fixed later but I'll add a comment in the code and a 
note on my todolist.

>
>  > +}
>  > +
>  > +inline Value *
>  > +NVC0LoweringPass::loadUboLength32(Value *ptr, uint32_t off)
>  > +{
>  > +   return loadResLength32(ptr, off, prog->driver->io.uboInfoBase);
>  > +}
>  > +
>  > +inline Value *
>  >  NVC0LoweringPass::loadMsInfo32(Value *ptr, uint32_t off)
>  >  {
>  >     uint8_t b = prog->driver->io.msInfoCBSlot;
>  > @@ -1711,7 +1729,42 @@ NVC0LoweringPass::handleLDST(Instruction *i)
>  >           assert(prog->getType() != Program::TYPE_FRAGMENT); // INTERP
>  >        }
>  >     } else if (i->src(0).getFile() == FILE_MEMORY_CONST) {
>  > -      if (i->src(0).isIndirect(1)) {
>  > +      if (targ->getChipset() >= NVISA_GK104_CHIPSET &&
>  > +          prog->getType() == Program::TYPE_COMPUTE) {
>  > +         // The launch descriptor only allows to set up 8 CBs, but
> OpenGL
>  > +         // requires at least 12 UBOs. To bypass this limitation, we
> store the
>  > +         // addrs into the driver constbuf and we directly load from
> the global
>  > +         // memory.
>  > +         if (i->src(0).isIndirect(1) || i->getSrc(0)->reg.fileIndex
>  > 0) {
>  > +            int8_t fileIndex = i->getSrc(0)->reg.fileIndex - 1;
>  > +            Value *ind = i->getIndirect(0, 1);
>  > +            Value *ptr = loadUboInfo64(ind, fileIndex * 16);
>  > +            if (i->src(0).isIndirect(1)) {
>  > +               Value *offset = bld.loadImm(NULL,
> i->getSrc(0)->reg.data.offset + typeSizeof(i->sType));
>  > +               Value *length = loadUboLength32(ind, fileIndex * 16);
>  > +               Value *pred = new_LValue(func, FILE_PREDICATE);
>  > +               if (i->src(0).isIndirect(0)) {
>  > +                  bld.mkOp2(OP_ADD, TYPE_U64, ptr, ptr,
> i->getIndirect(0, 0));
>  > +                  bld.mkOp2(OP_ADD, TYPE_U32, offset, offset,
> i->getIndirect(0, 0));
>  > +               }
>  > +               i->getSrc(0)->reg.file = FILE_MEMORY_GLOBAL;
>  > +               i->setIndirect(0, 1, NULL);
>  > +               i->setIndirect(0, 0, ptr);
>  > +               bld.mkCmp(OP_SET, CC_GT, TYPE_U32, pred, TYPE_U32,
> offset, length);
>  > +               i->setPredicate(CC_NOT_P, pred);
>  > +               if (i->defExists(0)) {
>  > +                  bld.mkMov(i->getDef(0), bld.mkImm(0));
>  > +               }
>  > +            } else {
>  > +               if (i->src(0).isIndirect(0)) {
>  > +                  bld.mkOp2(OP_ADD, TYPE_U64, ptr, ptr,
> i->getIndirect(0, 0));
>  > +               }
>  > +               i->getSrc(0)->reg.file = FILE_MEMORY_GLOBAL;
>  > +               i->setIndirect(0, 1, NULL);
>  > +               i->setIndirect(0, 0, ptr);
>  > +            }
>  > +         }
>
> This whole block is very confusing. You're checking the same conditions
> over and over.... Is there a clearer way of writing this logic? With
> fewer branches?

Yeah, this code looks a bit ugly. Will try to improve it.

>
>  > +      } else if (i->src(0).isIndirect(1)) {
>  >           Value *ptr;
>  >           if (i->src(0).isIndirect(0))
>  >              ptr = bld.mkOp3v(OP_INSBF, TYPE_U32, bld.getSSA(),
>  > diff --git
> a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
>  > index be81d29..aa19249 100644
>  > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
>  > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
>  > @@ -127,6 +127,9 @@ private:
>  >     Value *loadBufInfo32(Value *ptr, uint32_t off);
>  >     Value *loadBufInfo64(Value *ptr, uint32_t off);
>  >     Value *loadBufLength32(Value *ptr, uint32_t off);
>  > +   Value *loadUboInfo32(Value *ptr, uint32_t off);
>  > +   Value *loadUboInfo64(Value *ptr, uint32_t off);
>  > +   Value *loadUboLength32(Value *ptr, uint32_t off);
>  >     Value *loadMsInfo32(Value *ptr, uint32_t off);
>  >     Value *loadTexHandle(Value *ptr, unsigned int slot);
>  >
>  > --
>  > 2.7.4
>  >
>  > _______________________________________________
>  > mesa-dev mailing list
>  > mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>  > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

-- 
-Samuel