[Mesa-dev] [PATCH] i965/compaction: Avoid (unexpected) unsigned division.

[Matt Turner]

On Sat, Sep 27, 2014 at 8:09 AM, Jordan Justen <jljusten at gmail.com> wrote:
> On Fri, Sep 26, 2014 at 6:09 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> ... which leads to incorrect results on 32-bit x86.
>>
>> Reported-by: Mark Janes <mark.a.janes at intel.com>
>> ---
>> I tried writing up a nice commit message that explained what was going
>> on and why this worked on 64-bit, but then I realized that it was taking
>> orders of magnitude longer than the fix itself and probably no one would
>> care anyway.
>
> No cliff notes version? :)

Type promotion rules promote the int in 'int / sizeof(...)' to unsigned,
since sizeof(...) is unsigned.  Since the particular sizeofs in question
are powers of two, the compiler optimizes this into right shifts.

Unfortunately, since the possibly negative numerator has now been
promoted to unsigned, the compiler will choose a logical shift right
instruction rather than arithmetic.

On x86-64 gcc generates this code (before):

   sar    $0x20,%rax    ; int jump = brw_inst_imm_d(brw, insn);
   mov    %rax,%rsi
   shr    $0x3,%rsi     ; int jump_compacted = jump / sizeof(brw_compact_inst);
   shr    $0x4,%rax     ; int jump_uncompacted = jump / sizeof(brw_inst);

(after):

   shr    $0x20,%rax    ; ; int jump = brw_inst_imm_d(brw, insn);
   ; some exception case handling, and %eax copied to %ecx
   sar    $0x3,%ecx     ; int jump_compacted = jump / sizeof(brw_compact_inst);
   sar    $0x4,%eax     ; int jump_uncompacted = jump / sizeof(brw_inst);

On 64-bit platforms, there's no functional difference in this case,
since the high 32-bits of the 64-bit register are zero, but on 32-bit
platforms (this is how far I got, but this part seems unclear to me)

Basically, the unsigned promotion causes gcc to choose shr rather than
sar, and for some reasons that I haven't quite wrapped my head around
this doesn't cause problems on x86-64, but does on x86-32.

> Is it something that you could give a few lines of code that reproduce it?

I tried, and couldn't. :(

> Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

I've sent two patches that replace this one that hopefully make the
code a lot clearer.