[Mesa-dev] [PATCH 09/14] i965/compaction: Add support for Gen5.

[Kenneth Graunke]

On Tuesday, September 23, 2014 01:25:55 PM Matt Turner wrote:
> On Tue, Sep 23, 2014 at 1:10 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> > On Thu, Aug 28, 2014 at 8:10 PM, Matt Turner <mattst88 at gmail.com> wrote:
> >> +            int jump = brw_inst_imm_d(brw, insn);
> >> +            int jump_compacted = jump / sizeof(brw_compact_inst);
> >> +            int jump_uncompacted = jump / sizeof(brw_inst);
> >> +
> >> +            target_old_ip = this_old_ip + jump_uncompacted;
> >> +            target_compacted_count = compacted_counts[target_old_ip];
> >> +            jump_compacted -= (target_compacted_count -
> >> this_compacted_count);
> >> +            brw_inst_set_imm_ud(brw, insn, jump_compacted *
> >> +                                           sizeof(brw_compact_inst));
> >
> >
> > Any reason why you're reading it as a signed value and then writing it back
> > in unsigned?
> 
> IIRC brw_inst_set_imm_d() is broken. It's been long enough that I've
> forgotten the exact details, but _ud sets the exact bits you pass it,
> while _d does something wrong.

static inline void
brw_inst_set_imm_d(const struct brw_context *brw,
                   brw_inst *insn, int value)
{
   (void) brw;
   return brw_inst_set_bits(insn, 127, 96, value);
}

The final parameter of brw_inst_set_bits is a uint64_t, so it looks like the "int" value will get sign-extended to a 64-bit value, then treated as unsigned, which is now too wide to fit in bits 127:96.  IOW, it will never work for negative values...while, ironically, brw_inst_set_imm_ud() will.  (The int would be converted to unsigned...still 32-bit...then promoted from unsigned to uint64_t, which doesn't sign extend.)

That said, fixing it would be trivial:

   return brw_inst_set_bits(insn, 127, 96, (unsigned) value);

We should either fix it or delete it.  Either is fine by me.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140924/858417f4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140924/858417f4/attachment.sig>