[Mesa-dev] [PATCH 2/5] nv50/ir: make use of auxCBSlot instead of magic numbers

[Pierre Moreau]

Hello,

On 10:38 AM - Mar 16 2016, Hans de Goede wrote:
> Hi,
> 
> On 15-03-16 21:55, Samuel Pitoiset wrote:
> >This avoids using magic numbers for the driver constbuf slot which
> >is always 15 except for compute shaders on gk104+ where the slot 0
> >is used.
> >
> >For gk104+, some special compute-related values like the thread
> >index are uploaded to screen->parm which is currently bound on c0.
> >
> >Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> >---
> >  src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp     | 3 ++-
> >  src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> >index d284446..4bebfdc 100644
> >--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> >+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> >@@ -2178,7 +2178,8 @@ Converter::getResourceBase(const int r)
> >
> >     switch (r) {
> >     case TGSI_RESOURCE_GLOBAL:
> >-      sym = new_Symbol(prog, nv50_ir::FILE_MEMORY_GLOBAL, 15);
> >+      sym = new_Symbol(prog, nv50_ir::FILE_MEMORY_GLOBAL,
> >+                       info->io.auxCBSlot);
> 
> Note this is dead code, see patch 6/6 of the patch-set I just send.
> 
> Also do we need to specify a slot here at all? The new code paths
> to re-enable global mem with clover do not use this and work fine
> in my testing on a gk107.

On Fermi+, the global memory is united, but it is not the case on Tesla. So the
slot remains needed for Tesla. (Tesla will keep hunting you! :-p)

Global and const memory are not completely the same, so I'm somewhat reluctant
to reuse the same variable slot for both of them. But, I'm just giving my
2cents here, so feel free to keep as is.

Regardless of your pick, this is
Acked-by: Pierre Moreau <pierre.morrow at free.fr>

> 
> 
> >        break;
> >     case TGSI_RESOURCE_LOCAL:
> >        assert(prog->getType() == Program::TYPE_COMPUTE);
> >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 d879339..e0af4c0 100644
> >--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> >+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> >@@ -1698,7 +1698,8 @@ NVC0LoweringPass::handleRDSV(Instruction *i)
> >        }
> >        addr += prog->driver->prop.cp.gridInfoBase;
> >        bld.mkLoad(TYPE_U32, i->getDef(0),
> >-                 bld.mkSymbol(FILE_MEMORY_CONST, 0, TYPE_U32, addr), NULL);
> >+                 bld.mkSymbol(FILE_MEMORY_CONST, prog->driver->io.auxCBSlot,
> >+                              TYPE_U32, addr), NULL);
> >        break;
> 
> You're changing functionality here not just replacing a magic number,
> the commit msg does not reflect this. Maybe do this in a separate patch ?

Why is it changing functionality? This code is only run for GK104+, in which
case `prog->driver->io.auxCBSlot == 0`, cf patch 1.

Regards,
Pierre

> 
> Regards,
> 
> Hans
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160317/c9621ce6/attachment.sig>