[Mesa-dev] [PATCH v2] nv50/ir: make a copy of tex src if it's referenced multiple times
Ilia Mirkin
imirkin at alum.mit.edu
Tue Apr 10 11:50:51 UTC 2018
On Tue, Apr 10, 2018 at 6:08 AM, Karol Herbst <kherbst at redhat.com> wrote:
> I guess this fixes a bug somewhere?
Yeah... I describe it in the commit description, I thought. Here's the
situation:
%r1 = 5
%r2 = texsize %r1
%r3 = texsize %r1
Now, let's not worry about why those didn't get CSE'd. (Let's say they
refer to different textures, but query the same LOD.)
With the current code, r1, r2, and r3 all get joined to a single RIG
node with coalesceValues() which happens as part of the whole
JOIN_MASK_TEX thing (to make sure that src == dst regs for a tex op,
since nothing else can be encoded for nv50).
This is obviously bad - no way to make that RA happen -- the assigned
reg (to the RIG node) will overwrite the %r1 value after the first
texsize, and the second size will get a bogus LOD input in addition to
then also overwriting the result of the first texsize.
This is basically the same problem as a merge, and we get out of it
the same way as a merge -- adding extra copies. I refactored that
constraint code, although in hindsight perhaps I should have left it
alone and just pushed the tex onto the constrList and treat it like a
MERGE. I can go redo it that way too.
>
> 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?
There can only be a predicate on a def in SSA mode on UNION inputs,
not merge inputs (where this is a requirement).
>
>> - 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