[Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size
On Tesla cards, the first register $r0 contains the thread id; later generations use a specialised register for it. In order to prevent the register from being given to anyone, and thus lose the thread id information, an lvalue is created to represent $r0 and is passed as an argument to the `main` function. However, since the inputs and outputs of a function are stored as value definitions, a definition is added onto the previously created lvalue without it being associated to an instruction. Therefore, checking the number of definitions of an lvalue do not ensure that it is associated to an instruction. Fixes a nullptr dereference in the register allocation pass, while running compute kernels that do not use $r0. Signed-off-by: Pierre Moreau --- src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp index d877c25..500ab89 100644 --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp @@ -853,7 +853,7 @@ isShortRegOp(Instruction *insn) static bool isShortRegVal(LValue *lval) { - if (lval->defs.size() == 0) + if (lval->getInsn() == NULL) return false; for (Value::DefCIterator def = lval->defs.begin(); def != lval->defs.end(); ++def) @@ -1467,7 +1467,7 @@ GCRA::allocateRegisters(ArrayList& insns) nodes[i].init(regs, lval); RIG.insert(&nodes[i]); - if (lval->inFile(FILE_GPR) && lval->defs.size() > 0 && + if (lval->inFile(FILE_GPR) && lval->getInsn() != NULL && prog->getTarget()->getChipset() < 0xc0) { Instruction *insn = lval->getInsn(); if (insn->op == OP_MAD || insn->op == OP_SAD) -- 2.7.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size
This seems like correct. Reviewed-by: Samuel Pitoiset On 02/25/2016 02:03 AM, Pierre Moreau wrote: On Tesla cards, the first register $r0 contains the thread id; later generations use a specialised register for it. In order to prevent the register from being given to anyone, and thus lose the thread id information, an lvalue is created to represent $r0 and is passed as an argument to the `main` function. However, since the inputs and outputs of a function are stored as value definitions, a definition is added onto the previously created lvalue without it being associated to an instruction. Therefore, checking the number of definitions of an lvalue do not ensure that it is associated to an instruction. Fixes a nullptr dereference in the register allocation pass, while running compute kernels that do not use $r0. Signed-off-by: Pierre Moreau --- src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp index d877c25..500ab89 100644 --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp @@ -853,7 +853,7 @@ isShortRegOp(Instruction *insn) static bool isShortRegVal(LValue *lval) { - if (lval->defs.size() == 0) + if (lval->getInsn() == NULL) return false; for (Value::DefCIterator def = lval->defs.begin(); def != lval->defs.end(); ++def) @@ -1467,7 +1467,7 @@ GCRA::allocateRegisters(ArrayList& insns) nodes[i].init(regs, lval); RIG.insert(&nodes[i]); - if (lval->inFile(FILE_GPR) && lval->defs.size() > 0 && + if (lval->inFile(FILE_GPR) && lval->getInsn() != NULL && prog->getTarget()->getChipset() < 0xc0) { Instruction *insn = lval->getInsn(); if (insn->op == OP_MAD || insn->op == OP_SAD) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size
Patch is fine, description is wrong (or at least inaccurate). The real issue is that function arguments have defs, but no defining instruction. As a result, there's nothing to do when allocating registers. This has nothing to do with $r0, but it does have something to do with the fact that nv50 compute makes use of function arguments for compute programs. -ilia On Wed, Feb 24, 2016 at 8:03 PM, Pierre Moreau wrote: > On Tesla cards, the first register $r0 contains the thread id; later > generations use a specialised register for it. In order to prevent the > register > from being given to anyone, and thus lose the thread id information, an lvalue > is created to represent $r0 and is passed as an argument to the `main` > function. > > However, since the inputs and outputs of a function are stored as value > definitions, a definition is added onto the previously created lvalue without > it being associated to an instruction. Therefore, checking the number of > definitions of an lvalue do not ensure that it is associated to an > instruction. > > Fixes a nullptr dereference in the register allocation pass, while running > compute kernels that do not use $r0. > > Signed-off-by: Pierre Moreau > --- > src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > index d877c25..500ab89 100644 > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > @@ -853,7 +853,7 @@ isShortRegOp(Instruction *insn) > static bool > isShortRegVal(LValue *lval) > { > - if (lval->defs.size() == 0) > + if (lval->getInsn() == NULL) >return false; > for (Value::DefCIterator def = lval->defs.begin(); > def != lval->defs.end(); ++def) > @@ -1467,7 +1467,7 @@ GCRA::allocateRegisters(ArrayList& insns) > nodes[i].init(regs, lval); > RIG.insert(&nodes[i]); > > - if (lval->inFile(FILE_GPR) && lval->defs.size() > 0 && > + if (lval->inFile(FILE_GPR) && lval->getInsn() != NULL && > prog->getTarget()->getChipset() < 0xc0) { > Instruction *insn = lval->getInsn(); > if (insn->op == OP_MAD || insn->op == OP_SAD) > -- > 2.7.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
Re: [Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size
On 06:10 PM - Mar 08 2016, Ilia Mirkin wrote: > Patch is fine, description is wrong (or at least inaccurate). > > The real issue is that function arguments have defs, but no defining > instruction. As a result, there's nothing to do when allocating > registers. This has nothing to do with $r0, but it does have something > to do with the fact that nv50 compute makes use of function arguments > for compute programs. That's what I meant to write, but reading it again, it is confusing. I'll rewrite it. Pierre > > -ilia > > On Wed, Feb 24, 2016 at 8:03 PM, Pierre Moreau wrote: > > On Tesla cards, the first register $r0 contains the thread id; later > > generations use a specialised register for it. In order to prevent the > > register > > from being given to anyone, and thus lose the thread id information, an > > lvalue > > is created to represent $r0 and is passed as an argument to the `main` > > function. > > > > However, since the inputs and outputs of a function are stored as value > > definitions, a definition is added onto the previously created lvalue > > without > > it being associated to an instruction. Therefore, checking the number of > > definitions of an lvalue do not ensure that it is associated to an > > instruction. > > > > Fixes a nullptr dereference in the register allocation pass, while running > > compute kernels that do not use $r0. > > > > Signed-off-by: Pierre Moreau > > --- > > src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > > b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > > index d877c25..500ab89 100644 > > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp > > @@ -853,7 +853,7 @@ isShortRegOp(Instruction *insn) > > static bool > > isShortRegVal(LValue *lval) > > { > > - if (lval->defs.size() == 0) > > + if (lval->getInsn() == NULL) > >return false; > > for (Value::DefCIterator def = lval->defs.begin(); > > def != lval->defs.end(); ++def) > > @@ -1467,7 +1467,7 @@ GCRA::allocateRegisters(ArrayList& insns) > > nodes[i].init(regs, lval); > > RIG.insert(&nodes[i]); > > > > - if (lval->inFile(FILE_GPR) && lval->defs.size() > 0 && > > + if (lval->inFile(FILE_GPR) && lval->getInsn() != NULL && > > prog->getTarget()->getChipset() < 0xc0) { > > Instruction *insn = lval->getInsn(); > > if (insn->op == OP_MAD || insn->op == OP_SAD) > > -- > > 2.7.1 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev