Skip to content

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Aug 20, 2020

Adds these functions to toggle the output:

  • ccall(:jl_dump_emitted_mi_name, Nothing, (Ptr{Nothing},), e_io.handle)
  • ccall(:jl_dump_llvm_opt, Nothing, (Ptr{Nothing},), l_io.handle)

This will then be controlled from SnoopCompileCore, via timholy/SnoopCompile.jl#135.

This logs a CSV file with mappings from C function name to Julia
MethodInstance name, and a YAML file with timings statistics about the
LLVM module being optimized, including:

  • Total time to optimize the llvm module
  • The optlevel used for the llvm module
  • The name of each function in the module, before and after
  • Number of instructions for each function, before and after
  • Number of basic blocks for each function, before and after

Co-Authored-By: @Sacha0 @vchuravy
Fixes #31616

@NHDaly
Copy link
Member Author

NHDaly commented Aug 20, 2020

CC: @Sacha0 @vchuravy

@NHDaly NHDaly force-pushed the nhd-emit-per-method-llvm_timings branch 2 times, most recently from 7dd40b8 to d51dd2d Compare August 21, 2020 00:54
@NHDaly NHDaly force-pushed the nhd-emit-per-method-llvm_timings branch from 164f712 to 3b70a4a Compare August 21, 2020 22:32
NHDaly added a commit to NHDaly/julia that referenced this pull request Aug 23, 2020
@NHDaly
Copy link
Member Author

NHDaly commented Aug 23, 2020

(I've opened this branch which just applies these same changes back to julia 1.5, so we can test our codebase which currently doesn't support 1.6 because of some of our package dependencies):
v1.5.0...NHDaly:nhd-emit-per-method-llvm_timings--1.5

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Member Author

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, @vtjnash! Sorry for my delay in responding.

As a short-term bandaid, -O1 on julia 1.5 made a big improvement, so we stopped looking at this. But now that we're working through the inference timings, we'd like to go back to -O2, so we'll pick up this investigation again soon.

I'm sorry I let this languish for a bit, but do you think we'll be able to get this merged into 1.6? It would be really great to have this there so that our engineers don't need a custom build of julia to debug compilation times.

@NHDaly NHDaly marked this pull request as ready for review October 5, 2020 15:12
@NHDaly
Copy link
Member Author

NHDaly commented Oct 5, 2020

And, similarly, I'm going to try to wrap up the per-method-instance inference timings PR as well: #37749.

Ideally we'll be able to merge both of these this week so that they can make it into the v1.6 release! 😊 Does that seem possible to you? Sorry to be coming in so last-minute! ❤️ thanks for your help

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch looks great! Thanks @NHDaly! :)

It looks like one comment from Jameson remains outstanding (rewriting a conditional in terms of isDeclaration and startsWith), which I noted applies to similar code later in the patch as well. (Perhaps the duplicated code snippet should be factored into a little helper function? (It's sufficiently little duplication that I don't mind either way.) Otherwise this patch looks ready to go! :)

@NHDaly NHDaly force-pushed the nhd-emit-per-method-llvm_timings branch from 471adb7 to b01bb5e Compare October 10, 2020 03:31
@NHDaly
Copy link
Member Author

NHDaly commented Oct 10, 2020

Thanks @Sacha0! Addressed comments, rebased on top of master, and pushed.

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks @NHDaly! :)

@Sacha0 Sacha0 requested review from vchuravy and vtjnash October 10, 2020 14:31
@vchuravy
Copy link
Member

PR needs squashing.

Adds these functions to toggle the output:
  - ccall(:jl_dump_emitted_mi_name, Nothing, (Ptr{Nothing},), e_io.handle)
  - ccall(:jl_dump_llvm_opt, Nothing, (Ptr{Nothing},), l_io.handle)

This will then be controlled from SnoopCompileCore, via timholy/SnoopCompile.jl#135.

This logs a CSV file with mappings from C function name to Julia
MethodInstance name, and a YAML file with timings statistics about the
LLVM module being optimized, including:
 - Total time to optimize the llvm module
 - The `optlevel` used for the llvm module
 - The name of each function in the module, before and after
 - Number of instructions for each function, before and after
 - Number of basic blocks for each function, before and after

Co-Authored-By: Sacha Verweij <[email protected]>
Co-Authored-By: Valentin Churavy <[email protected]>
@NHDaly NHDaly force-pushed the nhd-emit-per-method-llvm_timings branch from b01bb5e to 1178f57 Compare October 12, 2020 18:27
@NHDaly
Copy link
Member Author

NHDaly commented Oct 12, 2020

Done. Thanks! :)

@vchuravy vchuravy merged commit f9ee161 into JuliaLang:master Oct 12, 2020
@NHDaly NHDaly deleted the nhd-emit-per-method-llvm_timings branch October 20, 2020 21:57
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.

4 participants