- Notifications
You must be signed in to change notification settings - Fork 817
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 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 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
/realloc
aroundtrace_{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