[Mesa-dev] [PATCH] nvc0/ir: fix robustness guarantees for constbuf loads on kepler+ compute

[Ilia Mirkin]

On Wed, Jan 25, 2017 at 11:51 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> Kepler and up unfortunately only support up to 8 constbufs. We work
> around this by loading from constbufs as if they were storage buffers.
> However we were not consistently applying limits to loads from these
> buffers. Make sure to do the same thing we do for storage buffers.
>
> Fixes GL45-CTS.robust_buffer_access_behavior.uniform_buffer
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  .../nouveau/codegen/nv50_ir_lowering_nvc0.cpp      | 46 ++++++++++------------
>  1 file changed, 21 insertions(+), 25 deletions(-)
>
> 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 cbea468..b3008b3 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> @@ -2289,32 +2289,28 @@ NVC0LoweringPass::handleLDST(Instruction *i)
>                               bld.loadImm(NULL, 12));
>           }
>
> -         if (i->src(0).isIndirect(1)) {
> -            Value *offset = bld.loadImm(NULL, i->getSrc(0)->reg.data.offset + typeSizeof(i->sType));
> -            Value *ptr = loadUboInfo64(ind, fileIndex * 16);
> -            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 (fileIndex >= 0) {
> -            Value *ptr = loadUboInfo64(ind, fileIndex * 16);
> -            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);
> +         assert(fileIndex >= 0 || ind);

Actually this assert can obviously fire (well, not obviously from this
context, but a few lines up...). It'll happen when accessing CONST[0]
which is the "non-ubo" constbuf. I've fixed this by instead doing

+         if (!ind && fileIndex == -1)
+            return;

> +
> +         Value *offset = bld.loadImm(NULL, i->getSrc(0)->reg.data.offset + typeSizeof(i->sType));
> +         Value *ptr = loadUboInfo64(ind, fileIndex * 16);
> +         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);
> +         Value *zero, *dst = i->getDef(0);
> +         i->setDef(0, bld.getSSA());
> +
> +         bld.setPosition(i, true);
> +         bld.mkMov((zero = bld.getSSA()), bld.mkImm(0))
> +            ->setPredicate(CC_P, pred);
> +         bld.mkOp2(OP_UNION, TYPE_U32, dst, i->getDef(0), zero);
>        } else if (i->src(0).isIndirect(1)) {
>           Value *ptr;
>           if (i->src(0).isIndirect(0))
> --
> 2.10.2
>