-
Notifications
You must be signed in to change notification settings - Fork 831
Add instrument allocations pass #6619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add instrument allocations pass #6619
Conversation
| (local.get $1) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option rather than instrumenting around each function call is to add new wrapper functions. That would be better for code size (existing calls remain the same size, they just point to the wrappers, and the wrappers are just a fixed increase in size). The inliner can inline them if the user then optimizes (and the inliner thinks it is worth it), which could make it just as fast.
But if size/speed are not important here then the current approach may be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally only intend to use it for debugging and not in the production code (at at least so far) so neither speed nor size is critical. I think the pass can be updated once there are different usecases.
b00c3a4 to
007b403
Compare
007b403 to
6666dd6
Compare
6666dd6 to
919df07
Compare
kripken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an explanation of the commandline format somewhere? If it's short enough in the --help that would be good, but I think it might not be, so the .cpp file perhaps.
However, a second question: Why does the commandline format have the types in there? It seems like we can detect the types from the function name? But I may have forgotten something here.
With the latest update where I removed the need for specifying the types, I think it's simple enough; there was already a help string defined in the TraceCalls.cpp in the
That's an excelent point; so initially the tracer was for pre-defined functions (malloc,calloc, realloc, free) but I changed that after the suggestion in the comment #6619 (comment) to make the pass more generic; I also blindly followed the recommendation to include types as parameters but it actually is not needed, and we can infer the type from the function being traced. I updated the PR now, thanks. |
c48e9d5 to
d79bb54
Compare
kripken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better!
d79bb54 to
290409f
Compare
290409f to
a69c304
Compare
a69c304 to
79e6d62
Compare
|
Looks like there are some CI errors that need to be resolved before landing (edit: i see there was a force-push since, let's see if that fixes things edit edit: still some errors it seems) |
13cb175 to
8120a53
Compare
8120a53 to
892ebf9
Compare
|
@kripken the build is fixed now. Tests were failing because I iterated through |
|
@loganek Iterating on a map here makes sense, yeah, as the keys are Names. In general we sometimes fix nondeterminism that way, if there is a natural key to use. If there aren't, say if the map is from |
This pass wraps
malloc/calloc/free/reallocaroundtrace_{func_name}functions so we can keep track on all the allocations. I understand this pass won't work for some of the languages, but opening a PR in case someone finds it useful and worth enough including in wasm-opt.Also, I don't have a ton of experience writing compiler passes, so any suggestion is very appreciated.
Related issue: #6548