- Notifications
You must be signed in to change notification settings - Fork 817
[DebugInfo] Copy debug info in call-utils.h #6652
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
Conversation
src/ir/debuginfo.h Outdated
inline void copyDebugInfoToReplacement(Expression* replacement, | ||
Expression* original, |
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.
Nit: How about putting the original
first? Source first and then the destination seems more intuitive to me, but if you like the current order it's fine too.
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.
Good point, yeah, this was not that clear. I reversed the order now and also renamed it so it is totally unambiguous, copyOriginalToReplacement
. I also removed DebugInfo
from the name as they are always called as debuginfo::copyFoo()
anyhow, so it is clear we are copying debuginfo.
;; CHECK: (func $call_ref-to-select (type $5) (param $x i32) (param $y i32) (param $z i32) (param $f (ref $i32_i32_=>_none)) | ||
;; CHECK-NEXT: (local $4 i32) | ||
;; CHECK-NEXT: (local $5 i32) | ||
;; CHECK-NEXT: ;;@ file.cpp:20:2 |
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.
How did these block
s and local.get
s get the debug info? The code only seems to put the debug function for the new calls...
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.
The outer block gets it because we replace the original instruction with the block, so the debuginfo is copied by default in replaceCurrent
(which seems like a reasonable default, and I don't think it is harmful here). But I am actually not sure why the local.get
also receives it... we must have specific code for that somewhere. It also seems ok to me though.
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.
But the debug info was already being copied in replaceCurrent
, no? I wonder what caused the change.
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.
Ok, I think I figured it out: these local.get
s are not actually new. They are reused from the original code before this optimization, and they all had debuginfo before, so they still have it after. CallUtils adds some additional local.get
s, which was confusing, and those have no debug info as expected.
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.
Specifically $x,$y,$z
are pre-existing locals and gets, with names and debug info, and $4,$5
are new locals with neither names nor debug 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.
But the debug info was already being copied in
replaceCurrent
, no? I wonder what caused the change.
But why did the block
get new debug info then? replaceCurrent
was already coping the debug info. Also I still don’t get why local.get
s that already existed newly got debug info. If those local.get
s already had debug info why was it not shown before? Other than makeCall
, this PR does not change other things semantically, no?
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.
The block gets debug info after this PR because the test had no debug info before. See line 310. Maybe a separate NFC PR that just modified the test would have made that clearer, sorry.
The local.get
s had debug info before, but by default we don't repeat debug info in the printed output:
;;@ file.cpp:20:2 (call_ref $i32_i32_=>_none (local.get $x) (local.get $y) (select (ref.func $foo) (ref.func $bar) (local.get $z) ) )
Not only the call_ref
has that debug location but all its children. We just don't print it (unless BINARYEN_PRINT_FULL=1
is set), because typically nested children have the same locations, so this makes the default text easier to read.
(If a child had different debug info we'd print that, or if it was marked as having no debug info we'd add ;;@
, with nothing else on that line.)
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.
Ah I see. Also I missed the newly added debug info. Thanks for the explanation!
We automatically copy debuginfo in
replaceCurrent()
, but there are a fewplaces that do other operations than simple replacements.
call-utils.h
willturn a
call_ref
with aselect
target into two directcall
s, and we were missingthe logic to copy debuginfo from the
call_ref
to thecall
s.To make this work, refactor out the copying logic from
wasm-traversal
, intodebuginfo.h
, and use it incall-utils.h
.debuginfo.h
itself is renamed fromdebug.h
(as now this needs to be includedfrom
wasm-traversal
, which nearly everything does, and it turns out some fileshave internal stuff like a
debug()
helper that ends up conflicing with the olddebug
namespace).Also rename the old
copyDebugInfo
function tocopyDebugInfoBetweenFunctions
which is more explicit. That is also moved from the header to a cpp file because
it depends on
wasm-traversal
(so we'd end up with recursive headers otherwise).That is fine, as that method is called after copying a function, which is not that
frequent. The new
copyDebugInfoToReplacement
(which was refactored out ofwasm-traversal
) is in the header because it can be called very frequently (everysingle instruction we optimize) and we want it to get inlined.