[Mesa-dev] [PATCH v2] nv50/ir: make a copy of tex src if it's referenced multiple times
Karol Herbst
kherbst at redhat.com
Tue Apr 10 10:08:35 UTC 2018
I guess this fixes a bug somewhere?
On Tue, Apr 10, 2018 at 6:11 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> For nv50 we coalesce the srcs and defs into a single node. As such, we
> can end up with impossible constraints if the source is referenced
> after the tex operation (which, due to the coalescing of values, will
> have overwritten it).
>
> This logic already exists for inserting moves for MERGE/UNION sources.
> It's the exact same idea here, so leverage that code, which also
> includes a few optimizations around not extending live ranges
> unnecessarily.
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>
> v1 -> v2: make use of existing logic in insertConstraintMoves
>
> src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp | 86 ++++++++++++----------
> 1 file changed, 49 insertions(+), 37 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp
> index 3a0e56e1385..7d107aca68d 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp
> @@ -257,6 +257,7 @@ private:
> private:
> virtual bool visit(BasicBlock *);
>
> + void insertConstraintMove(Instruction *, int s);
> bool insertConstraintMoves();
>
> void condenseDefs(Instruction *);
> @@ -2216,6 +2217,8 @@ RegAlloc::InsertConstraintsPass::texConstraintNV50(TexInstruction *tex)
> for (c = 0; tex->srcExists(c) || tex->defExists(c); ++c) {
> if (!tex->srcExists(c))
> tex->setSrc(c, new_LValue(func, tex->getSrc(0)->asLValue()));
> + else
> + insertConstraintMove(tex, c);
> if (!tex->defExists(c))
> tex->setDef(c, new_LValue(func, tex->getDef(0)->asLValue()));
> }
> @@ -2288,6 +2291,51 @@ RegAlloc::InsertConstraintsPass::visit(BasicBlock *bb)
> return true;
> }
>
> +void
> +RegAlloc::InsertConstraintsPass::insertConstraintMove(Instruction *cst, int s)
> +{
> + const uint8_t size = cst->src(s).getSize();
> +
> + assert(cst->getSrc(s)->defs.size() == 1); // still SSA
> +
> + Instruction *defi = cst->getSrc(s)->defs.front()->getInsn();
> + bool imm = defi->op == OP_MOV &&
> + defi->src(0).getFile() == FILE_IMMEDIATE;
> + bool load = defi->op == OP_LOAD &&
> + defi->src(0).getFile() == FILE_MEMORY_CONST &&
> + !defi->src(0).isIndirect(0);
> + // catch some cases where don't really need MOVs
> + if (cst->getSrc(s)->refCount() == 1 && !defi->constrainedDefs()) {
> + if (imm || load) {
> + // Move the defi right before the cst. No point in expanding
> + // the range.
> + defi->bb->remove(defi);
> + cst->bb->insertBefore(cst, defi);
> + }
> + return;
> + }
> +
> + LValue *lval = new_LValue(func, cst->src(s).getFile());
> + lval->reg.size = size;
> +
> + Instruction *mov = new_Instruction(func, OP_MOV, typeOfSize(size));
> + mov->setDef(0, lval);
> + mov->setSrc(0, cst->getSrc(s));
> +
> + if (load) {
> + mov->op = OP_LOAD;
> + mov->setSrc(0, defi->getSrc(0));
> + } else if (imm) {
> + mov->setSrc(0, defi->getSrc(0));
> + }
> +
> + if (defi->getPredicate())
> + mov->setPredicate(defi->cc, defi->getPredicate());
> +
> + cst->setSrc(s, mov->getDef(0));
> + cst->bb->insertBefore(cst, mov);
> +}
> +
> // Insert extra moves so that, if multiple register constraints on a value are
> // in conflict, these conflicts can be resolved.
> bool
> @@ -2328,46 +2376,10 @@ RegAlloc::InsertConstraintsPass::insertConstraintMoves()
> cst->bb->insertBefore(cst, mov);
> continue;
> }
> - assert(cst->getSrc(s)->defs.size() == 1); // still SSA
> -
> - Instruction *defi = cst->getSrc(s)->defs.front()->getInsn();
> - bool imm = defi->op == OP_MOV &&
> - defi->src(0).getFile() == FILE_IMMEDIATE;
> - bool load = defi->op == OP_LOAD &&
> - defi->src(0).getFile() == FILE_MEMORY_CONST &&
> - !defi->src(0).isIndirect(0);
> - // catch some cases where don't really need MOVs
> - if (cst->getSrc(s)->refCount() == 1 && !defi->constrainedDefs()) {
> - if (imm || load) {
> - // Move the defi right before the cst. No point in expanding
> - // the range.
> - defi->bb->remove(defi);
> - cst->bb->insertBefore(cst, defi);
> - }
> - continue;
> - }
>
> - LValue *lval = new_LValue(func, cst->src(s).getFile());
> - lval->reg.size = size;
> -
> - mov = new_Instruction(func, OP_MOV, typeOfSize(size));
> - mov->setDef(0, lval);
> - mov->setSrc(0, cst->getSrc(s));
> -
> - if (load) {
> - mov->op = OP_LOAD;
> - mov->setSrc(0, defi->getSrc(0));
> - } else if (imm) {
> - mov->setSrc(0, defi->getSrc(0));
> - }
> -
> - cst->setSrc(s, mov->getDef(0));
> - cst->bb->insertBefore(cst, mov);
> + insertConstraintMove(cst, s);
>
> cst->getDef(0)->asLValue()->noSpill = 1; // doesn't help
> -
> - if (cst->op == OP_UNION)
I am actually a bit wondering about this check we had and you removed.
Why shouldn't the predicate be set for OP_MERGE and why is it okay
after your change?
> - mov->setPredicate(defi->cc, defi->getPredicate());
> }
> }
> }
> --
> 2.16.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list