[Mesa-dev] [PATCH] intel/eu: print bytes instead of 32 bit hex value

[Sagar Ghuge]

On 08/20/2018 11:06 AM, Matt Turner wrote:
> Cool. This looks pretty good to me. A few comments inline.
> 
> On Wed, Aug 15, 2018 at 2:00 PM Sagar Ghuge <sagar.ghuge at intel.com> wrote:
>>
>> INTEL_DEBUG=hex prints 32 bit hex value
>> and due to endianness of CPU byte order is
>> reversed. In order to disassemble binary
>> files, print each byte instead of 32 bit hex
>> value.
> 
> Let's get your editor configured to line wrap at the correct length
> (these lines are too short).
> 
> If you use vim, you should be able to automatically line wrap to the
> appropriate length by highlighting the lines and then giving the
> command 'gq'
> 
>> Signed-off-by: Sagar Ghuge <sagar.ghuge at intel.com>
>> ---
>>  src/intel/compiler/brw_eu.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_eu.c b/src/intel/compiler/brw_eu.c
>> index 6ef0a6a577..223e561dff 100644
>> --- a/src/intel/compiler/brw_eu.c
>> +++ b/src/intel/compiler/brw_eu.c
>> @@ -365,9 +365,14 @@ brw_disassemble(const struct gen_device_info *devinfo,
>>        if (compacted) {
>>           brw_compact_inst *compacted = (void *)insn;
>>          if (dump_hex) {
>> -           fprintf(out, "0x%08x 0x%08x                       ",
>> -                   ((uint32_t *)insn)[1],
>> -                   ((uint32_t *)insn)[0]);
>> +           unsigned char * insn_ptr = ((unsigned char *)&insn[0]);
>> +           for (int i = 0 ; i < 8; i = i + 4) {
>> +              fprintf(out, "%02x %02x %02x %02x ",
>> +                      insn_ptr[i],
>> +                      insn_ptr[i + 1],
>> +                      insn_ptr[i + 2],
>> +                      insn_ptr[i + 3]);
>> +           }
> 
> I like printing the spaces between the bytes. That really shows more
> clearly that this is a byte array and not subject to any endianness
> issues.
> 
> One suggestion: let's print some blank spaces after the compacted
> instruction hex so that the disassembled instruction vertically aligns
> with uncompacted instructions. Currently we get disassembly that looks
> like
>

Thanks for reviewing the patch. Yes, I made changes and sent v2 according to
your suggestions.    
 
> 01 0b 1d 20 00 7c 02 00 mov(8)          g124<1>F        g2.3<0,1,0>F
> 01 00 60 00 e8 3a a0 2f 5c 00 00 00 00 00 00 00 mov(8)
> g125<1>F        g2.7<0,1,0>F
> 
> Also, we don't use tabs in i965. When editing old lines that had tabs,
> let's take the opportunity to remove them.
> 
> My ~/.vimrc has
> 
> autocmd BufNewFile,BufRead /home/mattst88/projects/mesa/* set
> expandtab tabstop=8 softtabstop=3 shiftwidth=3
> autocmd BufNewFile,BufRead
> /home/mattst88/projects/mesa/src/glsl/glcpp/* set noexpandtab
> tabstop=8 softtabstop=8 shiftwidth=8
> autocmd BufNewFile,BufRead
> /home/mattst88/projects/mesa/src/glsl/glsl_parser.yy set noexpandtab
> tabstop=8 shiftwidth=8
> autocmd BufNewFile,BufRead /home/mattst88/projects/piglit/* set
> noexpandtab tabstop=8 softtabstop=8 shiftwidth=8
> 
> to configure it appropriately for my Mesa and piglit directories.
> 

Thanks for sharing vimrc. I think my struggle ends here about getting coding style correct :)

> With those couple of small nits fixed, this will earn my review.
>