Skip to content

Conversation

kripken
Copy link
Member

@kripken kripken commented Jun 11, 2024

We automatically copy debuginfo in replaceCurrent(), but there are a few
places that do other operations than simple replacements. call-utils.h will
turn a call_ref with a select target into two direct calls, and we were missing
the logic to copy debuginfo from the call_ref to the calls.

To make this work, refactor out the copying logic from wasm-traversal, into
debuginfo.h, and use it in call-utils.h.

debuginfo.h itself is renamed from debug.h (as now this needs to be included
from wasm-traversal, which nearly everything does, and it turns out some files
have internal stuff like a debug() helper that ends up conflicing with the old
debug namespace).

Also rename the old copyDebugInfo function to copyDebugInfoBetweenFunctions
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 of
wasm-traversal) is in the header because it can be called very frequently (every
single instruction we optimize) and we want it to get inlined.

@kripken kripken requested a review from aheejin June 11, 2024 22:55
Comment on lines 28 to 29
inline void copyDebugInfoToReplacement(Expression* replacement,
Expression* original,
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

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

How did these blocks and local.gets get the debug info? The code only seems to put the debug function for the new calls...

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.gets 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.gets, which was confusing, and those have no debug info as expected.

Copy link
Member Author

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.

Copy link
Member

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.gets that already existed newly got debug info. If those local.gets already had debug info why was it not shown before? Other than makeCall, this PR does not change other things semantically, no?

Copy link
Member Author

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.gets 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.)

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. Also I missed the newly added debug info. Thanks for the explanation!

@kripken kripken merged commit ac21c8c into WebAssembly:main Jun 12, 2024
@kripken kripken deleted the call.debug branch June 12, 2024 18:05
kripken added a commit that referenced this pull request Jul 2, 2024
…ence (#6709) Previously the replacement select got the debug info, but we should also copy it to the values, as often optimizations lead to one of those values remaining by itself. Similar to #6652 in general form.
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants