[Mesa-dev] [PATCH 5/5] i965/fs: Debug the optimization passes by dumping instr to file.

[Matt Turner]

On Sun, Apr 13, 2014 at 12:16 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 04/11/2014 10:29 PM, Matt Turner wrote:
>> With INTEL_DEBUG=optimizer, write the output of dump_instructions() to a
>> file each time an optimization pass makes progress. This lets you easily
>> diff successive files to see what an optimization pass did.
>>
>> Example filenames written when running glxgears:
>>    fs8-00-00-start
>>    fs8-00-01-04-opt_copy_propagate
>>    fs8-00-01-06-dead_code_eliminate
>>    fs8-00-01-12-compute_to_mrf
>>    fs8-00-02-06-dead_code_eliminate
>>        |   |  |   |
>>        |   |  |   `-- optimization pass name
>>        |   |  |
>>        |   |  `-- optimization pass number in the loop
>>        |   |
>>        |   `-- optimization loop interation
>>        |
>>        `-- shader program number
>>
>> Note that with INTEL_DEBUG=optimizer, we disable compact_virtual_grfs,
>> so that we can diff instruction lists across loop interations without
>> the register numbers being changes.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 56 +++++++++++++++++++++++++++---------
>>  1 file changed, 43 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index c1d79e1..666cfa5b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -1704,6 +1704,9 @@ fs_visitor::split_virtual_grfs()
>>  void
>>  fs_visitor::compact_virtual_grfs()
>>  {
>> +   if (unlikely(INTEL_DEBUG & DEBUG_OPTIMIZER))
>> +      return;
>> +
>>     /* Mark which virtual GRFs are used, and count how many. */
>>     int remap_table[this->virtual_grf_count];
>>     memset(remap_table, -1, sizeof(remap_table));
>> @@ -3258,25 +3261,52 @@ fs_visitor::run()
>>
>>        opt_drop_redundant_mov_to_flags();
>>
>> +#define OPT(pass, args...)                                                 \
>> +   ({                                                                      \
>> +      pass_num++;                                                          \
>> +      bool progress = pass(args);                                          \
>> +                                                                           \
>> +      if (unlikely(INTEL_DEBUG & DEBUG_OPTIMIZER) && progress) {           \
>> +         char filename[64];                                                \
>> +         snprintf(filename, 64, "fs%d-%02d-%02d-%02d-" #pass,              \
>> +                  dispatch_width, shader_prog->Name, iteration, pass_num); \
>> +                                                                           \
>> +         backend_visitor::dump_instructions(filename);                                      \
>> +      }                                                                    \
>> +                                                                           \
>> +      progress;                                                            \
>> +   })
>
> I like the use of the macro.  But...folding in the "progress = ... ||
> progress" might be nice too.  It would also let us stick to standard C
> here, rather than GNU extensions.

I don't see a problem with using extensions, especially when we
already use them. But decluttering the loop seems like a good idea.

>> +
>> +      if (unlikely(INTEL_DEBUG & DEBUG_OPTIMIZER)) {
>> +         char filename[64];
>> +         snprintf(filename, 64, "fs%d-%02d-00-start",
>> +                  dispatch_width, shader_prog->Name);
>
> I am really unexcited about fixed length buffers, although you did do
> snprintf, and it'll probably not be a problem in practice.  Why not just
> asprintf or ralloc_asprintf it and then free it?  It's no more code and
> will just work.

Given the lengths of our optimization pass names, 64 seems like
plenty, and if it's not that worst that happens is we lose a few
characters from our optimization pass's name. My compiler complains
about not checking the return type of asprintf, and adding a memory
allocation failure check in the macro felt really stupid.