Skip to content
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 option to print inlining costs #160

Merged
merged 6 commits into from
Jul 22, 2021
Merged

add option to print inlining costs #160

merged 6 commits into from
Jul 22, 2021

Conversation

simeonschaub
Copy link
Collaborator

This allows printing inlining costs alongside optimized IR similar to
Base.print_statement_costs, which I found useful for debugging why
some functions are not inlined. They are highlighted green, because I
found them hard to distinguish otherwise, but I am open to suggestions
regarding formatting. This relies on JuliaLang/julia#41257, which I hope
can be backported to 1.7, so we don't have to worry about compatibility.

This allows printing inlining costs alongside optimized IR similar to
`Base.print_statement_costs`, which I found useful for debugging why
some functions are not inlined. They are highlighted green, because I
found them hard to distinguish otherwise, but I am open to suggestions
regarding formatting. This relies on JuliaLang/julia#41257, which I hope
can be backported to 1.7, so we don't have to worry about compatibility.
@simeonschaub
Copy link
Collaborator Author

An example of what this looks like:
Screenshot_2021-06-23_14-38-17

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #160 (35f4b50) into master (e954597) will decrease coverage by 0.20%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   44.61%   44.40%   -0.21%     
==========================================
  Files           7        7              
  Lines         901      912      +11     
==========================================
+ Hits          402      405       +3     
- Misses        499      507       +8     
Impacted Files Coverage Δ
src/Cthulhu.jl 18.75% <0.00%> (-0.19%) ⬇️
src/ui.jl 0.00% <0.00%> (ø)
src/codeview.jl 47.11% <68.96%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e954597...35f4b50. Read the comment docs.

timholy added a commit to aviatesk/juliacon2021-workshop-pkgdev that referenced this pull request Jul 22, 2021
This makes a minor changes to the README:
- removes the part about accepting nominations (no time left...)
- expands the installation documentation
- proposes that we split the SnoopCompile part into "pre-Cthulhu" and "post-Cthulhu"

This also adds a couple more packages (JET, SnoopCompile) and adds [compat] for long-term compatibility.

There does not seem to be a suitable Cthulhu release; either we'll need to make a branch or we can get a release out before Saturday. I favor the latter, if possible.
JuliaDebug/Cthulhu.jl#160 looks delicious and would be nice to merge if it wouldn't jeopardize the existing functionality.
@aviatesk aviatesk self-requested a review July 22, 2021 10:45
aviatesk pushed a commit to aviatesk/juliacon2021-workshop-pkgdev that referenced this pull request Jul 22, 2021
* Update the README and add [compat]

This makes a minor changes to the README:
- removes the part about accepting nominations (no time left...)
- expands the installation documentation
- proposes that we split the SnoopCompile part into "pre-Cthulhu" and "post-Cthulhu"

This also adds a couple more packages (JET, SnoopCompile) and adds [compat] for long-term compatibility.

There does not seem to be a suitable Cthulhu release; either we'll need to make a branch or we can get a release out before Saturday. I favor the latter, if possible.
JuliaDebug/Cthulhu.jl#160 looks delicious and would be nice to merge if it wouldn't jeopardize the existing functionality.

* update Manifest to julia 1.7
Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

Nice feature !

end
println(io)

if src isa IRCode && inline_cost
Copy link
Member

Choose a reason for hiding this comment

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

We can just allow the inlining cost printing for unoptimized source as well ?

Suggested change
if src isa IRCode && inline_cost
if inline_cost

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did actually experiment with that, but the issue is that any call that isn't an intrinsic is assumed to be a dynamic call, so it's probably more misleading than useful. I wonder whether we can get the recursive total costs after inlining for each call from the AbstractInterpreter, but that is probably a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see.
For now maybe it'd be more kind if we print some warning message like:

        elseif toggle === :optimize
            optimize ⊻= true
            if !is_cached(mi)
                @warn "can't switch to post-optimization state, since this inference frame isn't cached"
                optimize ⊻= true
            end
            continue
        elseif toggle === :debuginfo
            debuginfo = DebugInfo((Int(debuginfo) + 1) % 3)
        elseif toggle === :inline_cost
            if !optimize
                @warn "enable optimization to see the inlining costs"
            end
            inline_cost ⊻= true

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that sounds reasonable. Do you just want to push that to this branch?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I will push the change and merge this.

@aviatesk aviatesk merged commit 8e96c04 into master Jul 22, 2021
@aviatesk aviatesk deleted the sds/show_inlining branch July 22, 2021 17:56
@timholy timholy mentioned this pull request Jul 28, 2021
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.

3 participants