[Mesa-dev] [PATCH 3/6] nv50/ir: optimize neg(add(bool, 1)) to bool for OP_SET and OP_SLCT

[Ilia Mirkin]

On Mon, Jan 25, 2016 at 9:57 AM, Karol Herbst <nouveau at karolherbst.de> wrote:
> From: Karol Herbst <git at karolherbst.de>
>
> helps shaders in saints row IV, bioshock infinite and shadow warrior
>
> total instructions in shared programs : 1922121 -> 1911112 (-0.57%)
> total gprs used in shared programs    : 251878 -> 251739 (-0.06%)
> total local used in shared programs   : 5673 -> 5673 (0.00%)
> total bytes used in shared programs   : 17624144 -> 17523440 (-0.57%)
>
>                 local        gpr       inst      bytes
>     helped           0         137         720         720
>       hurt           0           8           0           0
>
> Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
> ---
>  .../drivers/nouveau/codegen/nv50_ir_peephole.cpp   | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> index 9c60ea1..8dc0844 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> @@ -1510,6 +1510,7 @@ private:
>     void handleCVT_CVT(Instruction *);
>     void handleCVT_EXTBF(Instruction *);
>     void handleSUCLAMP(Instruction *);
> +   void handleNEG(Instruction *);
>
>     BuildUtil bld;
>  };
> @@ -1982,6 +1983,30 @@ AlgebraicOpt::handleSUCLAMP(Instruction *insn)
>     insn->setSrc(0, add->getSrc(s));
>  }
>

Please add a comment here explaining what this opt is doing.

> +void
> +AlgebraicOpt::handleNEG(Instruction *i) {
> +   Value *src = i->getSrc(0);

You only ever refer to src->getInsn() -- why not make this a
Instruction *si = i->getSrc(0)->getInsn();

> +   if (src->getInsn()->op == OP_AND) {
> +      ImmediateValue immd;

Everywhere else we use "imm".

> +      int b = 1;
> +      if (!src->getInsn()->src(0).getImmediate(immd)) {
> +         if (!src->getInsn()->src(1).getImmediate(immd))
> +            return;
> +         b = 0;
> +      }

This is confusing. How about

if (si->src(0).getImmediate(imm))
  t = 1;
else if (si->src(1).getImmediate(imm))
  t = 0;
else
  return;

> +
> +      if (immd.isInteger(1)) {
> +         Value *srcAnd = src->getInsn()->getSrc(b);
> +         operation op = srcAnd->getInsn()->op;
> +         if (op == OP_SET || op == OP_SLCT) {

Not all selects produce boolean results. Only ones that return 0/-1 do.

Also a set might return a boolean float, aka 0/1.0, in which case this
is not what you want. If it's a set, make sure the dType is not
isFloatType().

  -ilia

> +            i->op = OP_MOV;
> +            i->setSrc(0, srcAnd);
> +            return;
> +         }
> +      }
> +   }
> +}
> +
>  bool
>  AlgebraicOpt::visit(BasicBlock *bb)
>  {
> @@ -2019,6 +2044,9 @@ AlgebraicOpt::visit(BasicBlock *bb)
>        case OP_SUCLAMP:
>           handleSUCLAMP(i);
>           break;
> +      case OP_NEG:
> +         handleNEG(i);
> +         break;
>        default:
>           break;
>        }
> --
> 2.7.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev