Re: [Mesa-dev] [PATCH 2/6] nv50/ir: swap sources in PostRaConstantFolding when src0 is imm

[Karol Herbst]

> Ilia Mirkin <imir...@alum.mit.edu> hat am 26. Januar 2016 um 04:53
> geschrieben:
> 
> On Mon, Jan 25, 2016 at 9:57 AM, Karol Herbst <nouv...@karolherbst.de> wrote:
> > From: Karol Herbst <g...@karolherbst.de>
> >
> > helps some shaders in multiple games
> >
> > total instructions in shared programs : 1922267 -> 1922121 (-0.01%)
> > total gprs used in shared programs : 251878 -> 251878 (0.00%)
> > total local used in shared programs : 5673 -> 5673 (0.00%)
> > total bytes used in shared programs : 17625496 -> 17624144 (-0.01%)
> >
> > local gpr inst bytes
> > helped 0 0 62 62
> > hurt 0 0 0 0
> >
> > Signed-off-by: Karol Herbst <nouv...@karolherbst.de>
> > ---
> > .../drivers/nouveau/codegen/nv50_ir_peephole.cpp | 69 +++++++++++++---------
> > 1 file changed, 42 insertions(+), 27 deletions(-)
> >
> > diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > index bfec130..9c60ea1 100644
> > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > @@ -2881,37 +2881,52 @@ NV50PostRaConstantFolding::visit(BasicBlock *bb)
> > def = i->getSrc(1)->getInsn();
> > if (def &amp;&amp; def->op == OP_SPLIT &amp;&amp; typeSizeof(def->sType) ==
> > 4)
> > def = def->getSrc(0)->getInsn();
> > - if (def &amp;&amp; def->op == OP_MOV &amp;&amp; def->src(0).getFile() ==
> > FILE_IMMEDIATE) {
> 
> This is a lot of diff of some very subtle code. But really what you want is
> 
> if (!def || def->op != MOV || !immediate) {
>  see if doing the swap is beneficial, do it, and reset def
> }
> 
> and then the rest of the code remains the same. Right?
> 

well the code changes, because the condition is checked in my change again and
there is no way that the old check can be false:

old:

if (def && def->op == OP_MOV && def->src(0).getFile() == FILE_IMMEDIATE) {
   // do stuff
}

new:
if (!def || def->op != OP_MOV || def->src(0).getFile() != FILE_IMMEDIATE) {
   def = // do stuff
   if (!def || def->op != OP_MOV || def->src(0).getFile() != FILE_IMMEDIATE)) //
<===== the old check revered
      break; // <===== leave the switch

   // swap the sources
}

if (def && def->op == OP_MOV && def->src(0).getFile() == FILE_IMMEDIATE) { //
<==== check not needed anymore because it is guarenteed by the above code
   // do the old stuff
}

I know the diff looks ugly, so maybe I do it in two patches for clarity?

> > - vtmp = i->getSrc(1);
> > - if (typeSizeof(i->sType) > 2) {
> > - i->setSrc(1, def->getSrc(0));
> > - } else {
> > - ImmediateValue val;
> > - bool ret = def->src(0).getImmediate(val);
> > - assert(ret);
> > - if (typeSizeof(i->sType) == 2) {
> > - if (i->getSrc(1)->reg.data.id &amp; 1)
> > - val.reg.data.u32 >>= 16;
> > - val.reg.data.u32 &amp;= 0xffff;
> > - }
> > - i->setSrc(1, new_ImmediateValue(bb->getProgram(), val.reg.data.u32));
> > - }
> > + if (!(def &amp;&amp; def->op == OP_MOV &amp;&amp; def->src(0).getFile() ==
> > FILE_IMMEDIATE)) {
> > + /* maybe we can swap it */
> > + def = i->getSrc(0)->getInsn();
> > + if (def &amp;&amp; def->op == OP_SPLIT &amp;&amp; typeSizeof(def->sType)
> > == 4)
> > + def = def->getSrc(0)->getInsn();
> > + if (!(def &amp;&amp; def->op == OP_MOV &amp;&amp; def->src(0).getFile() ==
> > FILE_IMMEDIATE))
> > + break;
> > +
> > + Value *tmpS = i->getSrc(0);
> > + Modifier tmpM = i->src(0).mod;
> > +
> > + i->setSrc(0, i->getSrc(1));
> > + i->src(0).mod = i->src(1).mod;
> >
> > - /* There's no post-RA dead code elimination, so do it here
> > - * XXX: if we add more code-removing post-RA passes, we might
> > - * want to create a post-RA dead-code elim pass */
> > - if (post_ra_dead(vtmp->getInsn())) {
> > - Value *src = vtmp->getInsn()->getSrc(0);
> > - // Careful -- splits will have already been removed from the
> > - // functions. Don't double-delete.
> > - if (vtmp->getInsn()->bb)
> > - delete_Instruction(prog, vtmp->getInsn());
> > - if (src->getInsn() &amp;&amp; post_ra_dead(src->getInsn()))
> > - delete_Instruction(prog, src->getInsn());
> > + i->setSrc(1, tmpS);
> > + i->src(1).mod = tmpM;
> > + }
> > +
> > + vtmp = i->getSrc(1);
> > + if (typeSizeof(i->sType) > 2) {
> > + i->setSrc(1, def->getSrc(0));
> > + } else {
> > + ImmediateValue val;
> > + bool ret = def->src(0).getImmediate(val);
> > + assert(ret);
> > + if (typeSizeof(i->sType) == 2) {
> > + if (i->getSrc(1)->reg.data.id &amp; 1)
> > + val.reg.data.u32 >>= 16;
> > + val.reg.data.u32 &amp;= 0xffff;
> > }
> > + i->setSrc(1, new_ImmediateValue(bb->getProgram(), val.reg.data.u32));
> > + }
> >
> > - break;
> > + /* There's no post-RA dead code elimination, so do it here
> > + * XXX: if we add more code-removing post-RA passes, we might
> > + * want to create a post-RA dead-code elim pass */
> > + if (post_ra_dead(vtmp->getInsn())) {
> > + Value *src = vtmp->getInsn()->getSrc(0);
> > + // Careful -- splits will have already been removed from the
> > + // functions. Don't double-delete.
> > + if (vtmp->getInsn()->bb)
> > + delete_Instruction(prog, vtmp->getInsn());
> > + if (src->getInsn() &amp;&amp; post_ra_dead(src->getInsn()))
> > + delete_Instruction(prog, src->getInsn());
> > }
> > +
> > break;
> > default:
> > break;
> > --
> > 2.7.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev