Page MenuHomePhabricator

Added line numbers to the debug output of PDL bytecode.
ClosedPublic

Authored by sfuniak on Tue, Nov 16, 10:59 PM.

Details

Summary

This is a small diff that splits out the debug output for PDL bytecode. When running bytecode with debug output on, it is useful to know the line numbers where the PDLIntepr operations are performed. Usually, these are in a single MLIR file, so it's sufficient to print out the line number rather than the entire location (which tends to be quite verbose). This debug output is gated by LLVM_DEBUG rather than #ifndef NDEBUG to make it easier to test.

Diff Detail

Event Timeline

sfuniak created this revision.Tue, Nov 16, 10:59 PM
sfuniak requested review of this revision.Tue, Nov 16, 10:59 PM
rriddle added inline comments.Thu, Nov 18, 10:54 AM
mlir/lib/Rewrite/ByteCode.cpp
711–712

What's the downside to adding in the filename? I've had some thoughts of building a stepthrough debugger for the bytecode and I'd like to have the filename, as the frontend I have (PDLL presented recently) allows include files).

825

Can we move all of these additions to functions/helpers that explicitly spell out what they mean? i.e. when I see the NoOp here it isn't clear to me what it is supposed to be used for, but we actually have a line number being consumed. Also, why do only some operations have this addition, but not others?

2011–2014

Can you move this to a helper function+add documentation? it isn't clear what this corresponds to.

sfuniak added inline comments.Thu, Nov 18, 2:56 PM
mlir/lib/Rewrite/ByteCode.cpp
711–712

Sure, we can add the filename.

825

The requirement would be that, in the debug mode, all pdl_interp operations generate some bytecode operation. So only those operations that did not produce bytecode now have a NoOp. But it's not ideal that with debug output on, we have a different set of bytecode operations. I will change the mechanism to avoid the NoOps.

sfuniak updated this revision to Diff 388373.Thu, Nov 18, 8:20 PM

Print location instead of line numbers.

rriddle added inline comments.Fri, Nov 19, 12:05 PM
mlir/lib/Rewrite/ByteCode.cpp
2011–2012

Can we avoid this? We end up paying the cost for every bytecode op. Could we extract the old -1 in executeForEach to something like getPrevCodeIt that adjusts for the location in debug mode?

sfuniak updated this revision to Diff 388774.Sun, Nov 21, 3:18 PM

Extract the previous code iterator manually.

rriddle accepted this revision.Wed, Nov 24, 12:39 PM

LG, thanks!

mlir/lib/Rewrite/ByteCode.cpp
2011

nit: Can you add a comment here?

This revision is now accepted and ready to land.Wed, Nov 24, 12:39 PM
sfuniak updated this revision to Diff 389901.Thu, Nov 25, 9:14 PM

Added comment.

This revision was automatically updated to reflect the committed changes.