Re: [Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size

[Pierre Moreau]

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([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

Re: [Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size

[Ilia Mirkin]

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([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

[Samuel Pitoiset]

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([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

[Mesa-dev] [PATCH] nv50/ir: Check for valid insn instead of defs size

[Pierre Moreau]

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([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