-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[self-profiler] add selfprofiling to llvm #68406
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
2077006
to
cf9a55e
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Thanks for the PR, @andjo403! I'll try to take a closer look at this some time later this week. |
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 have some minor feedback about the self-profiler changes. I don't know enough about LLVM to review those parts so I'll leave that to @michaelwoerister.
cf9a55e
to
4f4a71d
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
4f4a71d
to
de5daa8
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
9372209
to
84ac459
Compare
☔ The latest upstream changes (presumably #68474) made this pull request unmergeable. Please resolve the merge conflicts. |
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 did a first review pass. Looks great! I want to take a closer look before merging. For now, could you move the LLVM specific code to librustc_codegen_llvm
(as mentioned below).
let ir_name = ir_name_to_string_id(profiler, string_cache, part); | ||
components.push(StringComponent::Value(SEPARATOR_BYTE)); | ||
components.push(StringComponent::Ref(ir_name)); | ||
} |
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 ever more than one argument? If not you can use the EventIdBuilder::from_label_and_arg
.
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.
for LazyCallGraph::SCC there can be up to 10 args (if more they are truncated by llvm) do not know if it is interesting to see all the functions in the SCC (and in that case make a new getName function in llvm that returns all not only 10) or if it is enough to print the first function. see around https://github.com/rust-lang/llvm-project/blob/a6f4c1bb07d58df5956d2c49be68547143f77b18/llvm/include/llvm/Analysis/LazyCallGraph.h#L447 for more info.
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.
Yeah we'll want to add a EventIdBuilder::from_label_and_args(label: _, args: &[_])
method at some point anyway. For now encoding things directly is fine.
84ac459
to
968442b
Compare
have fixed the comments now |
shall there be some check or documentation that llvm event recording only works when |
That's a good idea and makes sense to me. I'm not sure we should error out in that case but a warning or similar seems appropriate to me. |
Yes, a warning makes sense to me too. |
how do I add a warning? was not that easy to find some other code that use it. |
You could use |
☔ The latest upstream changes (presumably #69030) made this pull request unmergeable. Please resolve the merge conflicts. |
968442b
to
1be179e
Compare
added a warning if the new passmanager is not used and llvm events is requested. |
☔ The latest upstream changes (presumably #69088) made this pull request unmergeable. Please resolve the merge conflicts. |
Placement of the warning seems fine to me |
1be179e
to
cec0ed0
Compare
the PR that this depended on have merged to master and I have now rebase onto master. |
@bors r+ rollup=never |
📌 Commit cec0ed0 has been approved by |
[self-profiler] add selfprofiling to llvm using pass name as event id and add additional data with name of module, function … ![image](https://user-images.githubusercontent.com/844398/72761970-205d8600-3bde-11ea-86de-87386e127944.png) r? @michaelwoerister or @wesleywiser
☀️ Test successful - checks-azure |
using pass name as event id and add additional data with name of module, function …
r? @michaelwoerister or @wesleywiser