I guess this fixes a bug somewhere? On Tue, Apr 10, 2018 at 6:11 AM, Ilia Mirkin <imir...@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 <imir...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev