Nouveau - May 2015 - [PATCH 1/4] nvc0/ir: avoid jumping to a sched instruction

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
Pretty sure there's nothing wrong with it, but it looks odd in the code.

 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp | 2 ++
 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp | 7 +++++--
 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp  | 2 ++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
index 6bb9620..28081fa 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
@@ -1316,6 +1316,8 @@ CodeEmitterGK110::emitFlow(const Instruction *i)
    } else
    if (mask & 2) {
       int32_t pcRel = f->target.bb->binPos - (codeSize + 8);
+      if (writeIssueDelays && !(f->target.bb->binPos & 0x3f))
+         pcRel += 8;
       // currently we don't want absolute branches
       assert(!f->absolute);
       code[0] |= (pcRel & 0x1ff) << 23;
diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
index 22db368..442cedf 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
@@ -509,10 +509,13 @@ CodeEmitterGM107::emitBRA()
    emitCond5(0x00, CC_TR);
 
    if (!insn->srcExists(0) || insn->src(0).getFile() !=
FILE_MEMORY_CONST) {
+      int32_t pos = insn->target.bb->binPos;
+      if (writeIssueDelays && !(pos & 0x1f))
+         pos += 8;
       if (!insn->absolute)
-         emitField(0x14, 24, insn->target.bb->binPos - (codeSize + 8));
+         emitField(0x14, 24, pos - (codeSize + 8));
       else
-         emitField(0x14, 32, insn->target.bb->binPos);
+         emitField(0x14, 32, pos);
    } else {
       emitCBUF (0x24, gpr, 20, 16, 0, insn->src(0));
       emitField(0x05, 1, 1);
diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
index d9aed34..c241973 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
@@ -1406,6 +1406,8 @@ CodeEmitterNVC0::emitFlow(const Instruction *i)
    } else
    if (mask & 2) {
       int32_t pcRel = f->target.bb->binPos - (codeSize + 8);
+      if (writeIssueDelays && !(f->target.bb->binPos & 0x3f))
+         pcRel += 8;
       // currently we don't want absolute branches
       assert(!f->absolute);
       code[0] |= (pcRel & 0x3f) << 26;
-- 
2.3.6

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp | 12 ++++++++----
 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp |  1 +
 src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp  |  8 +++++++-
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
index 28081fa..ab8bf2e 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gk110.cpp
@@ -967,8 +967,8 @@ CodeEmitterGK110::emitSET(const CmpInstruction *i)
       code[0] = (code[0] & ~0xfc) | ((code[0] << 3) & 0xe0);
       if (i->defExists(1))
          defId(i->def(1), 2);
-   else
-      code[0] |= 0x1c;
+      else
+         code[0] |= 0x1c;
    } else {
       switch (i->sType) {
       case TYPE_F32: op2 = 0x000; op1 = 0x800; break;
@@ -990,8 +990,12 @@ CodeEmitterGK110::emitSET(const CmpInstruction *i)
       }
       FTZ_(3a);
 
-      if (i->dType == TYPE_F32)
-         code[1] |= 1 << 23;
+      if (i->dType == TYPE_F32) {
+         if (isFloatType(i->sType))
+            code[1] |= 1 << 23;
+         else
+            code[1] |= 1 << 15;
+      }
    }
    if (i->sType == TYPE_S32)
       code[1] |= 1 << 19;
diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
index 442cedf..399a6f1 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
@@ -1830,6 +1830,7 @@ CodeEmitterGM107::emitISET()
    emitCond3(0x31, insn->setCond);
    emitField(0x30, 1, isSignedType(insn->sType));
    emitCC   (0x2f);
+   emitField(0x2c, 1, insn->dType == TYPE_F32);
    emitX    (0x2b);
    emitGPR  (0x08, insn->src(0));
    emitGPR  (0x00, insn->def(0));
diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
index c241973..f5992bc 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_nvc0.cpp
@@ -1078,8 +1078,14 @@ CodeEmitterNVC0::emitSET(const CmpInstruction *i)
    if (!isFloatType(i->sType))
       lo = 0x3;
 
-   if (isFloatType(i->dType) || isSignedIntType(i->sType))
+   if (isSignedIntType(i->sType))
       lo |= 0x20;
+   if (isFloatType(i->dType)) {
+      if (isFloatType(i->sType))
+         lo |= 0x20;
+      else
+         lo |= 0x80;
+   }
 
    switch (i->op) {
    case OP_SET_AND: hi = 0x10000000; break;
-- 
2.3.6

This has started to happen more now that the backend is producing
KILL_IF more often.

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
 .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 29 ++++++++++++++++++++++
 .../nouveau/codegen/nv50_ir_target_nv50.cpp        |  2 ++
 2 files changed, 31 insertions(+)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
index 14446b6..d8af19a 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
@@ -973,6 +973,35 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue
&imm0, int s)
    }
       break;
 
+   case OP_AND:
+   {
+      CmpInstruction *cmp = i->getSrc(t)->getInsn()->asCmp();
+      if (!cmp || cmp->op == OP_SLCT)
+         return;
+      if (!prog->getTarget()->isOpSupported(cmp->op, TYPE_F32))
+         return;
+      if (imm0.reg.data.f32 != 1.0)
+         return;
+      if (cmp == NULL)
+         return;
+      if (i->getSrc(t)->getInsn()->dType != TYPE_U32)
+         return;
+
+      i->getSrc(t)->getInsn()->dType = TYPE_F32;
+      if (i->src(t).mod != Modifier(0)) {
+         assert(i->src(t).mod == Modifier(NV50_IR_MOD_NOT));
+         i->src(t).mod = Modifier(0);
+         cmp->setCond = reverseCondCode(cmp->setCond);
+      }
+      i->op = OP_MOV;
+      i->setSrc(s, NULL);
+      if (t) {
+         i->setSrc(0, i->getSrc(t));
+         i->setSrc(t, NULL);
+      }
+   }
+      break;
+
    case OP_SHL:
    {
       if (s != 1 || i->src(0).mod != Modifier(0))
diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
index 178a167..70180eb 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
@@ -413,6 +413,8 @@ TargetNV50::isOpSupported(operation op, DataType ty) const
       return false;
    case OP_SAD:
       return ty == TYPE_S32;
+   case OP_SET:
+      return !isFloatType(ty);
    default:
       return true;
    }
-- 
2.3.6

This covers the pattern where a KILL_IF is used, which triggers a
comparison of -x to 0. This can usually be folded into the comparison whose
result is being compared to 0, however it may, itself, have already been
combined with another comparison. That shouldn't impact the logic of
this pass however. With this and the & 1.0 change, code like

00000020: 001c0001 80081df4     set b32 $r0 lt f32 $r0 0x3e800000
00000028: 001c0000 201fc000     and b32 $r0 $r0 0x3f800000
00000030: 7f9c001e dd885c00     set $p0 0x1 lt f32 neg $r0 0x0
00000038: 0000003c 19800000     $p0 discard

becomes

00000020: 001c001d b5881df4     set $p0 0x1 lt f32 $r0 0x3e800000
00000028: 0000003c 19800000     $p0 discard

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
 .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 51 ++++++++++++++--------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
index d8af19a..43a2fe9 100644
--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
@@ -278,7 +278,6 @@ private:
 
    void tryCollapseChainedMULs(Instruction *, const int s,
ImmediateValue&);
 
-   // TGSI 'true' is converted to -1 by F2I(NEG(SET)), track back to
SET
    CmpInstruction *findOriginForTestWithZero(Value *);
 
    unsigned int foldCount;
@@ -337,16 +336,10 @@ ConstantFolding::findOriginForTestWithZero(Value *value)
       return NULL;
    Instruction *insn = value->getInsn();
 
-   while (insn && insn->op != OP_SET) {
+   while (insn && insn->op != OP_SET && insn->op !=
OP_SET_AND &&
+          insn->op != OP_SET_OR && insn->op != OP_SET_XOR) {
       Instruction *next = NULL;
       switch (insn->op) {
-      case OP_NEG:
-      case OP_ABS:
-      case OP_CVT:
-         next = insn->getSrc(0)->getInsn();
-         if (insn->sType != next->dType)
-            return NULL;
-         break;
       case OP_MOV:
          next = insn->getSrc(0)->getInsn();
          break;
@@ -946,29 +939,51 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue
&imm0, int s)
 
    case OP_SET: // TODO: SET_AND,OR,XOR
    {
+      /* This optimizes the case where the output of a set is being compared
+       * to zero. Since the set can only produce 0/-1 (int) or 0/1 (float), we
+       * can be a lot cleverer in our comparison.
+       */
       CmpInstruction *si = findOriginForTestWithZero(i->getSrc(t));
       CondCode cc, ccZ;
-      if (i->src(t).mod != Modifier(0))
-         return;
-      if (imm0.reg.data.u32 != 0 || !si || si->op != OP_SET)
+      if (imm0.reg.data.u32 != 0 || !si)
          return;
       cc = si->setCond;
       ccZ = (CondCode)((unsigned int)i->asCmp()->setCond & ~CC_U);
+      // We do everything assuming var (cmp) 0, reverse the condition if 0 is
+      // first.
       if (s == 0)
          ccZ = reverseCondCode(ccZ);
+      // If there is a negative modifier, we need to undo that, by flipping
+      // the comparison to zero.
+      if (i->src(t).mod.neg())
+         ccZ = reverseCondCode(ccZ);
+      // If this is a signed comparison, we expect the input to be a regular
+      // boolean, i.e. 0/-1. However the rest of the logic assumes that true
+      // is positive, so just flip the sign.
+      if (i->sType == TYPE_S32) {
+         assert(!isFloatType(si->dType));
+         ccZ = reverseCondCode(ccZ);
+      }
       switch (ccZ) {
-      case CC_LT: cc = CC_FL; break;
-      case CC_GE: cc = CC_TR; break;
-      case CC_EQ: cc = inverseCondCode(cc); break;
-      case CC_LE: cc = inverseCondCode(cc); break;
-      case CC_GT: break;
-      case CC_NE: break;
+      case CC_LT: cc = CC_FL; break; // bool < 0 -- this is never true
+      case CC_GE: cc = CC_TR; break; // bool >= 0 -- this is always true
+      case CC_EQ: cc = inverseCondCode(cc); break; // bool == 0 -- !bool
+      case CC_LE: cc = inverseCondCode(cc); break; // bool <= 0 -- !bool
+      case CC_GT: break; // bool > 0 -- bool
+      case CC_NE: break; // bool != 0 -- bool
       default:
          return;
       }
+
+      // Update the condition of this SET to be identical to the origin set,
+      // but with the updated condition code. The original SET should get
+      // DCE'd, ideally.
+      i->op = si->op;
       i->asCmp()->setCond = cc;
       i->setSrc(0, si->src(0));
       i->setSrc(1, si->src(1));
+      if (si->srcExists(2))
+         i->setSrc(2, si->src(2));
       i->sType = si->sType;
    }
       break;
-- 
2.3.6

On 09.05.2015 07:35, Ilia Mirkin wrote:
> This has started to happen more now that the backend is producing
> KILL_IF more often.
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>   .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 29
++++++++++++++++++++++
>   .../nouveau/codegen/nv50_ir_target_nv50.cpp        |  2 ++
>   2 files changed, 31 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index 14446b6..d8af19a 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -973,6 +973,35 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue
&imm0, int s)
>      }
>         break;
>   
> +   case OP_AND:
> +   {
> +      CmpInstruction *cmp = i->getSrc(t)->getInsn()->asCmp();
> +      if (!cmp || cmp->op == OP_SLCT)
how about if (cmp == NULL || ...) and kill the same condition later?
> +         return;
> +      if (!prog->getTarget()->isOpSupported(cmp->op, TYPE_F32))
> +         return;
> +      if (imm0.reg.data.f32 != 1.0)
> +         return;
> +      if (cmp == NULL)
> +         return;
> +      if (i->getSrc(t)->getInsn()->dType != TYPE_U32)
> +         return;
> +
> +      i->getSrc(t)->getInsn()->dType = TYPE_F32;
> +      if (i->src(t).mod != Modifier(0)) {
> +         assert(i->src(t).mod == Modifier(NV50_IR_MOD_NOT));
> +         i->src(t).mod = Modifier(0);
> +         cmp->setCond = reverseCondCode(cmp->setCond);
> +      }
> +      i->op = OP_MOV;
> +      i->setSrc(s, NULL);
> +      if (t) {
> +         i->setSrc(0, i->getSrc(t));
> +         i->setSrc(t, NULL);
> +      }
> +   }
> +      break;
> +
>      case OP_SHL:
>      {
>         if (s != 1 || i->src(0).mod != Modifier(0))
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
> index 178a167..70180eb 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nv50.cpp
> @@ -413,6 +413,8 @@ TargetNV50::isOpSupported(operation op, DataType ty)
const
>         return false;
>      case OP_SAD:
>         return ty == TYPE_S32;
> +   case OP_SET:
> +      return !isFloatType(ty);
>      default:
>         return true;
>      }

On 09.05.2015 07:35, Ilia Mirkin wrote:
> This covers the pattern where a KILL_IF is used, which triggers a
> comparison of -x to 0. This can usually be folded into the comparison whose
> result is being compared to 0, however it may, itself, have already been
> combined with another comparison. That shouldn't impact the logic of
> this pass however. With this and the & 1.0 change, code like
>
> 00000020: 001c0001 80081df4     set b32 $r0 lt f32 $r0 0x3e800000
> 00000028: 001c0000 201fc000     and b32 $r0 $r0 0x3f800000
> 00000030: 7f9c001e dd885c00     set $p0 0x1 lt f32 neg $r0 0x0
> 00000038: 0000003c 19800000     $p0 discard
>
> becomes
>
> 00000020: 001c001d b5881df4     set $p0 0x1 lt f32 $r0 0x3e800000
> 00000028: 0000003c 19800000     $p0 discard
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>   .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 51
++++++++++++++--------
>   1 file changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index d8af19a..43a2fe9 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -278,7 +278,6 @@ private:
>   
>      void tryCollapseChainedMULs(Instruction *, const int s,
ImmediateValue&);
>   
> -   // TGSI 'true' is converted to -1 by F2I(NEG(SET)), track back
to SET
>      CmpInstruction *findOriginForTestWithZero(Value *);
>   
>      unsigned int foldCount;
> @@ -337,16 +336,10 @@ ConstantFolding::findOriginForTestWithZero(Value
*value)
>         return NULL;
>      Instruction *insn = value->getInsn();
>   
> -   while (insn && insn->op != OP_SET) {
> +   while (insn && insn->op != OP_SET && insn->op !=
OP_SET_AND &&
> +          insn->op != OP_SET_OR && insn->op != OP_SET_XOR) {
>         Instruction *next = NULL;
>         switch (insn->op) {
> -      case OP_NEG:
> -      case OP_ABS:
> -      case OP_CVT:
> -         next = insn->getSrc(0)->getInsn();
> -         if (insn->sType != next->dType)
> -            return NULL;
> -         break;
>         case OP_MOV:
>            next = insn->getSrc(0)->getInsn();
>            break;
> @@ -946,29 +939,51 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue
&imm0, int s)
>   
>      case OP_SET: // TODO: SET_AND,OR,XOR
delete this comment?!
>      {
> +      /* This optimizes the case where the output of a set is being
compared
> +       * to zero. Since the set can only produce 0/-1 (int) or 0/1
(float), we
> +       * can be a lot cleverer in our comparison.
> +       */
>         CmpInstruction *si = findOriginForTestWithZero(i->getSrc(t));
>         CondCode cc, ccZ;
> -      if (i->src(t).mod != Modifier(0))
> -         return;
> -      if (imm0.reg.data.u32 != 0 || !si || si->op != OP_SET)
> +      if (imm0.reg.data.u32 != 0 || !si)
>            return;
>         cc = si->setCond;
>         ccZ = (CondCode)((unsigned int)i->asCmp()->setCond &
~CC_U);
> +      // We do everything assuming var (cmp) 0, reverse the condition if 0
is
> +      // first.
>         if (s == 0)
>            ccZ = reverseCondCode(ccZ);
> +      // If there is a negative modifier, we need to undo that, by
flipping
> +      // the comparison to zero.
> +      if (i->src(t).mod.neg())
> +         ccZ = reverseCondCode(ccZ);
> +      // If this is a signed comparison, we expect the input to be a
regular
> +      // boolean, i.e. 0/-1. However the rest of the logic assumes that
true
> +      // is positive, so just flip the sign.
> +      if (i->sType == TYPE_S32) {
> +         assert(!isFloatType(si->dType));
> +         ccZ = reverseCondCode(ccZ);
> +      }
can both this and the previous condition evaluate to true? if yes, this 
double-flips ccZ...
>         switch (ccZ) {
> -      case CC_LT: cc = CC_FL; break;
> -      case CC_GE: cc = CC_TR; break;
> -      case CC_EQ: cc = inverseCondCode(cc); break;
> -      case CC_LE: cc = inverseCondCode(cc); break;
> -      case CC_GT: break;
> -      case CC_NE: break;
> +      case CC_LT: cc = CC_FL; break; // bool < 0 -- this is never true
> +      case CC_GE: cc = CC_TR; break; // bool >= 0 -- this is always
true
> +      case CC_EQ: cc = inverseCondCode(cc); break; // bool == 0 -- !bool
> +      case CC_LE: cc = inverseCondCode(cc); break; // bool <= 0 --
!bool
> +      case CC_GT: break; // bool > 0 -- bool
> +      case CC_NE: break; // bool != 0 -- bool
>         default:
>            return;
>         }
> +
> +      // Update the condition of this SET to be identical to the origin
set,
> +      // but with the updated condition code. The original SET should get
> +      // DCE'd, ideally.
> +      i->op = si->op;
>         i->asCmp()->setCond = cc;
>         i->setSrc(0, si->src(0));
>         i->setSrc(1, si->src(1));
> +      if (si->srcExists(2))
> +         i->setSrc(2, si->src(2));
>         i->sType = si->sType;
>      }
>         break;