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

DebugInfo: Add a method to find debug info for replacements and use it in Precompute #5900

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 24, 2023

Many optimizations just move code around, and debug info is preserved anyhow as we
do so. Precompute however constantly creates new constant nodes, so we were losing
a significant amount of debug info.

Just copying debug info for the replacement is not enough, as we may replace a parent
with a child, like this:

(block
  ;; debug info on the const
  (i32.const 42)
)
=>
(i32.const 42)

When replacing the block we should look for debug info anywhere inside it, if we have
anything else, that is, debug info on a child is probably better than not having any
debug info at all.

This helps save debug info after inlining in some cases, see the new testcases.

@kripken kripken requested a review from aheejin August 24, 2023 22:06
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #5900 (30a3df7) into main (0598939) will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #5900      +/-   ##
==========================================
+ Coverage   42.57%   42.58%   +0.01%     
==========================================
  Files         484      485       +1     
  Lines       74831    74845      +14     
  Branches    11924    11931       +7     
==========================================
+ Hits        31858    31876      +18     
+ Misses      39772    39762      -10     
- Partials     3201     3207       +6     
Files Changed Coverage Δ
src/ir/debug.h 85.71% <ø> (+85.71%) ⬆️
src/ir/debug.cpp 84.61% <84.61%> (ø)
src/passes/Precompute.cpp 80.00% <100.00%> (+0.25%) ⬆️

... and 7 files with indirect coverage changes

@aheejin
Copy link
Member

aheejin commented Aug 25, 2023

Is this generally helpful even when the replacement is not a child of the original? In case it is not, is inaccurate info better than no info?

@kripken
Copy link
Member Author

kripken commented Aug 25, 2023

I think it is generally helpful, but maybe we can think of an example that's not true? My thinking, at least, is that if we replace A(B, C) with some D then D does what all of A(B, C) did, so it's better to use the source file location for say C if we don't have one for A - it's at least representing the location of some of the source code that D implements.

@kripken
Copy link
Member Author

kripken commented Aug 30, 2023

This is probably still worth landing but I'll wait for other pending PRs in this area to land first, as this is the least important. We may also want a more general fix here, I'm not sure.

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.

None yet

2 participants