Skip to content

Print hashes in AST prettyprint output#478

Merged
tmcdonell merged 2 commits intoAccelerateHS:masterfrom
tomsmeding:print-hash-master
Jan 8, 2021
Merged

Print hashes in AST prettyprint output#478
tmcdonell merged 2 commits intoAccelerateHS:masterfrom
tomsmeding:print-hash-master

Conversation

@tomsmeding
Copy link
Copy Markdown
Member

@tomsmeding tomsmeding commented Nov 3, 2020

Description
In this PR, the prettyprinter for the AST is changed to annotate nodes with their hash. This is accomplished by creating a new PrettyConfig type that, for now, specifies just one configurable item: the formatting function for node names. In the future this could potentially include more settings (what comes to mind is a setting to output a best-effort approximation of valid Haskell input).

The reason why this is still a draft PR is that the hashes are currently printed unconditionally: we might want to disable this by default. However, I'm not sure how best to make this configurable (an environment variable feels ugly for this); what do you think?

Motivation and context
This is useful in particular when used together with AccelerateHS/accelerate-llvm#65, which prints the hash of the AST corresponding to a kernel in the -ddump-exec output. This then allows linking the timings in that debug output with the actual AST, which is useful for profiling larger Accelerate programs.

How has this been tested?
It looks right on the small and large example that I tested. :)

Types of changes
What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist
Go over all the following points, and put an x in all the boxes that apply. If you're unsure about any of these, don't hesitate to ask. We're here to help!

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@tmcdonell
Copy link
Copy Markdown
Member

We could add this under the option -dverbose?

@tomsmeding
Copy link
Copy Markdown
Member Author

I think that makes sense. Users that aren't interested in internals are not going to pass -dverbose, and users that want to see what's happening will need to use the debug flags anyway.

The only imperfectness I think is that -dverbose also does other things, since it interacts with other flags and produces extra output there. However, I believe the only thing it currently does in absence of other flags is pass -v to ptxas and print some additional debug info there. I think that's not too much, and not having granular flags for this is fine.

I'll have a look later today at making this conditional on the verbose flag, also in accelerate-llvm.

@tomsmeding tomsmeding marked this pull request as ready for review November 18, 2020 18:55
@tmcdonell tmcdonell merged commit e632752 into AccelerateHS:master Jan 8, 2021
@tmcdonell
Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants