[Mesa-dev] [PATCH] nouveau: codegen: combineLd/St do not combine indirect loads

[Ilia Mirkin]

On Thu, Apr 21, 2016 at 9:55 AM, Hans de Goede <hdegoede at redhat.com> wrote:
> combineLd/St would combine, i.e. :
>
> st  u32 # g[$r2+0x0] $r2
> st  u32 # g[$r2+0x4] $r3
>
> into:
>
> st  u64 # g[$r2+0x0] $r2d
>
> But this is only valid if r2 contains an 8 byte aligned address,
> which is unknown.
>
> This commit checks for src0 dim 0 not being indirect when combining
> loads / stores as combining indirect loads / stores may break alignment
> rules.

I believe the assumption is that all indirect addresses are 16-byte
aligned. This works out for GL, I think. Although hm... I wonder what
happens if you have a

layout (std430) buffer foo {
  int x[16];
}

And you access x[i], x[i+1], and i == 1. I think we end up doing a ton
of size-based validation which might avoid the problem.

My concern is that now constbufs will get the same treatment, and for
constbufs the alignment is always 16 :(

What do you think? Just drop those, or add extra conditionals to allow
it for constbufs?

  -ilia

>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index fea3886..0f8ccf8 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -2200,8 +2200,8 @@ MemoryOpt::combineLd(Record *rec, Instruction *ld)
>         isAccessSupported(ld->getSrc(0)->reg.file, typeOfSize(size)))
>        return false;
>     // no unaligned loads
> -   if (((size == 0x8) && (MIN2(offLd, offRc) & 0x7)) ||
> -       ((size == 0xc) && (MIN2(offLd, offRc) & 0xf)))
> +   if (((size == 0x8) && (rec->rel[0] || MIN2(offLd, offRc) & 0x7)) ||
> +       ((size == 0xc) && (rec->rel[0] || MIN2(offLd, offRc) & 0xf)))
>        return false;
>
>     assert(sizeRc + sizeLd <= 16 && offRc != offLd);
> @@ -2255,7 +2255,7 @@ MemoryOpt::combineSt(Record *rec, Instruction *st)
>     if (!prog->getTarget()->
>         isAccessSupported(st->getSrc(0)->reg.file, typeOfSize(size)))
>        return false;
> -   if (size == 8 && MIN2(offRc, offSt) & 0x7)
> +   if (size == 8 && (rec->rel[0] || MIN2(offRc, offSt) & 0x7))
>        return false;
>
>     st->takeExtraSources(0, extra); // save predicate and indirect address
> --
> 2.7.3
>