[Mesa-dev] [PATCH] nv50/ir: Properly fold constants in SPLIT operation

[Pierre Moreau]

I think we never split values that are < 64-bit wide, as the RA pass does not
support < 32-bit wide values. And 64-bit values aren’t used that often, I would
guess.

Does adding the Fixes: tag here work, or should I send a new version?

Fixes: b7d9677d ("nv50/ir: constant fold OP_SPLIT")

On 02:46 pm - Jun 21 2017, Ilia Mirkin wrote:
> Wait, this is actively buggy! How did this ever work :( I guess we
> don't split immediates too frequently, and I was testing it with
> zero's or something.
> 
> Can you figure out the commit where I added this idiotic code and add
> a Fixes: tag?
> 
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: mesa-stable at lists.freedesktop.org
> 
> On Mon, Jun 12, 2017 at 4:53 PM, Pierre Moreau <pierre.morrow at free.fr> wrote:
> > Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
> > ---
> >  src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > index e032255178..57223d311c 100644
> > --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
> > @@ -975,8 +975,9 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue &imm0, int s)
> >        bld.setPosition(i, false);
> >
> >        uint8_t size = i->getDef(0)->reg.size;
> > -      uint32_t mask = (1ULL << size) - 1;
> > -      assert(size <= 32);
> > +      uint8_t bitsize = size * 8;
> > +      uint32_t mask = (1ULL << bitsize) - 1;
> > +      assert(bitsize <= 32);
> >
> >        uint64_t val = imm0.reg.data.u64;
> >        for (int8_t d = 0; i->defExists(d); ++d) {
> > @@ -984,7 +985,7 @@ ConstantFolding::opnd(Instruction *i, ImmediateValue &imm0, int s)
> >           assert(def->reg.size == size);
> >
> >           newi = bld.mkMov(def, bld.mkImm((uint32_t)(val & mask)), TYPE_U32);
> 
> Is that what you want here? Should this be typeOfSize(size) ?
> 
> I guess you want to split this into 32-bit values anyways since that's
> what everything processes... eventually we might want to play around
> with the SIMD opcodes but probably not soon.
> 
> > -         val >>= size;
> > +         val >>= bitsize;
> >        }
> >        delete_Instruction(prog, i);
> >        break;
> > --
> > 2.13.1
> >