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.gh...@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.gh...@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. > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev