Skip to content

[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.

Sign up for GitHub

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

Merged
kripken merged 8 commits into from
Jun 12, 2024
Merged

[DebugInfo] Copy debug info in call-utils.h #6652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c9b3fde
work
kripken Jun 11, 2024
9e29331
work
kripken Jun 11, 2024
09ceb14
work
kripken Jun 11, 2024
91d1f95
test
kripken Jun 11, 2024
5b13148
format
kripken Jun 11, 2024
c9ad13f
unfo
kripken Jun 11, 2024
4025c83
rename
kripken Jun 12, 2024
285590d
format
kripken Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ir/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ FILE(GLOB ir_HEADERS *.h)
set(ir_SOURCES
ExpressionAnalyzer.cpp
ExpressionManipulator.cpp
debuginfo.cpp
drop.cpp
effects.cpp
eh-utils.cpp
Expand Down
21 changes: 8 additions & 13 deletions src/ir/debug.h → src/ir/debuginfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2019 WebAssembly Community Group participants
* Copyright 2024 WebAssembly Community Group participants
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -14,16 +14,13 @@
* limitations under the License.
*/

#ifndef wasm_ir_debug_h
#define wasm_ir_debug_h
#include "ir/debuginfo.h"
#include "wasm-traversal.h"
#include "wasm.h"

#include <wasm-traversal.h>
namespace wasm::debuginfo {

namespace wasm::debug {

// Given an expression and a copy of it in another function, copy the debug
// info into the second function.
inline void copyDebugInfo(Expression* origin,
void copyBetweenFunctions(Expression* origin,
Expression* copy,
Function* originFunc,
Function* copyFunc) {
Expand Down Expand Up @@ -52,8 +49,6 @@ inline void copyDebugInfo(Expression* origin,
copyDebug[copyList.list[i]] = location;
}
}
};

} // namespace wasm::debug
}

#endif // wasm_ir_debug_h
} // namespace wasm::debuginfo
72 changes: 72 additions & 0 deletions src/ir/debuginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2019 WebAssembly Community Group participants
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef wasm_ir_debuginfo_h
#define wasm_ir_debuginfo_h

#include "wasm.h"

namespace wasm::debuginfo {

// Given an original expression and another that replaces it, copy the debuginfo
// from the former to the latter. Note the expression may not be an exclusive
// replacement of the other (the other may be replaced by several expressions,
// all of whom may end up with the same debug info).
inline void copyOriginalToReplacement(Expression* original,
Expression* replacement,
Function* func) {
auto& debugLocations = func->debugLocations;
// Early exit if there is no debug info at all. Also, leave if we already
// have debug info on the new replacement, which we don't want to trample:
// if there is no debug info we do want to copy, as a replacement operation
// suggests the new code plays the same role (it is an optimized version of
// the old), but if the code is already annotated, trust that.
if (debugLocations.empty() || debugLocations.count(replacement)) {
return;
}

auto iter = debugLocations.find(original);
if (iter != debugLocations.end()) {
debugLocations[replacement] = iter->second;
// Note that we do *not* erase the debug info of the expression being
// replaced, because it may still exist: we might replace
//
// (call
// (block ..
//
// with
//
// (block
// (call ..
//
// We still want the call here to have its old debug info.
//
// (In most cases, of course, we do remove the replaced expression,
// which means we accumulate unused garbage in debugLocations, but
// that's not that bad; we use arena allocation for Expressions, after
// all.)
}
}

// Given an expression and a copy of it in another function, copy the debug
// info into the second function.
void copyBetweenFunctions(Expression* origin,
Expression* copy,
Function* originFunc,
Function* copyFunc);
} // namespace wasm::debuginfo

#endif // wasm_ir_debuginfo_h
4 changes: 2 additions & 2 deletions src/ir/module-utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

#include "module-utils.h"
#include "ir/debug.h"
#include "ir/debuginfo.h"
#include "ir/intrinsics.h"
#include "ir/manipulation.h"
#include "ir/properties.h"
Expand Down Expand Up @@ -54,7 +54,7 @@ Function* copyFunction(Function* func,
ret->localNames = func->localNames;
ret->localIndices = func->localIndices;
ret->body = ExpressionManipulator::copy(func->body, out);
debug::copyDebugInfo(func->body, ret->body, func, ret.get());
debuginfo::copyBetweenFunctions(func->body, ret->body, func, ret.get());
ret->prologLocation = func->prologLocation;
ret->epilogLocation = func->epilogLocation;
// Update file indices if needed
Expand Down
4 changes: 2 additions & 2 deletions src/passes/Inlining.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
#include <atomic>

#include "ir/branch-utils.h"
#include "ir/debug.h"
#include "ir/debuginfo.h"
#include "ir/drop.h"
#include "ir/eh-utils.h"
#include "ir/element-utils.h"
Expand Down Expand Up @@ -579,7 +579,7 @@ static Expression* doInlining(Module* module,

// Generate and update the inlined contents
auto* contents = ExpressionManipulator::copy(from->body, *module);
debug::copyDebugInfo(from->body, contents, from, into);
debuginfo::copyBetweenFunctions(from->body, contents, from, into);
updater.walk(contents);
block->list.push_back(contents);
block->type = retType;
Expand Down
14 changes: 9 additions & 5 deletions src/passes/call-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <variant>

#include "ir/debuginfo.h"
#include "ir/type-updating.h"
#include "wasm.h"

Expand Down Expand Up @@ -130,14 +131,17 @@ convertToDirectCalls(T* curr,
};

auto makeCall = [&](IndirectCallInfo info) -> Expression* {
Expression* ret;
if (std::get_if<Trap>(&info)) {
return builder.makeUnreachable();
ret = builder.makeUnreachable();
} else {
return builder.makeCall(std::get<Known>(info).target,
getOperands(),
curr->type,
curr->isReturn);
ret = builder.makeCall(std::get<Known>(info).target,
getOperands(),
curr->type,
curr->isReturn);
}
debuginfo::copyOriginalToReplacement(curr, ret, &func);
return ret;
};
auto* ifTrueCall = makeCall(ifTrueCallInfo);
auto* ifFalseCall = makeCall(ifFalseCallInfo);
Expand Down
33 changes: 3 additions & 30 deletions src/wasm-traversal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#ifndef wasm_wasm_traversal_h
#define wasm_wasm_traversal_h

#include "ir/debuginfo.h"
#include "support/small_vector.h"
#include "support/threads.h"
#include "wasm.h"
Expand Down Expand Up @@ -123,36 +124,8 @@ struct Walker : public VisitorType {
Expression* replaceCurrent(Expression* expression) {
// Copy debug info, if present.
if (currFunction) {
auto& debugLocations = currFunction->debugLocations;
// Early exit if there is no debug info at all. Also, leave if we already
// have debug info on the new expression, which we don't want to trample:
// if there is no debug info we do want to copy, as a replacement
// operation suggests the new code plays the same role (it is an optimized
// version of the old), but if the code is already annotated, trust that.
if (!debugLocations.empty() && !debugLocations.count(expression)) {
auto* curr = getCurrent();
auto iter = debugLocations.find(curr);
if (iter != debugLocations.end()) {
debugLocations[expression] = iter->second;
// Note that we do *not* erase the debug info of the expression being
// replaced, because it may still exist: we might replace
//
// (call
// (block ..
//
// with
//
// (block
// (call ..
//
// We still want the call here to have its old debug info.
//
// (In most cases, of course, we do remove the replaced expression,
// which means we accumulate unused garbage in debugLocations, but
// that's not that bad; we use arena allocation for Expressions, after
// all.)
}
}
debuginfo::copyOriginalToReplacement(
getCurrent(), expression, currFunction);
}
return *replacep = expression;
}
Expand Down
25 changes: 23 additions & 2 deletions test/lit/passes/optimize-instructions-call_ref.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,16 @@
)

;; CHECK: (func $call_ref-to-direct (type $i32_i32_=>_none) (param $x i32) (param $y i32)
;; CHECK-NEXT: ;;@ file.cpp:10:1
;; CHECK-NEXT: (call $foo
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $call_ref-to-direct (param $x i32) (param $y i32)
;; This call_ref should become a direct call.
;; This call_ref should become a direct call. The debuginfo should transfer as
;; well.
;;@ file.cpp:10:1
(call_ref $i32_i32_=>_none
(local.get $x)
(local.get $y)
Expand Down Expand Up @@ -253,29 +256,42 @@
;; 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

@aheejin aheejin Jun 12, 2024

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

@kripken kripken Jun 12, 2024

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

@aheejin aheejin Jun 12, 2024

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

@kripken kripken Jun 12, 2024

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

@kripken kripken Jun 12, 2024

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

@aheejin aheejin Jun 12, 2024

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

@kripken kripken Jun 12, 2024

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

@aheejin aheejin Jun 12, 2024

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!

;; CHECK-NEXT: (block
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (local.set $4
;; CHECK-NEXT: ;;@ file.cpp:20:2
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (local.set $5
;; CHECK-NEXT: ;;@ file.cpp:20:2
;; CHECK-NEXT: (local.get $y)
;; CHECK-NEXT: )
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (if
;; CHECK-NEXT: ;;@ file.cpp:20:2
;; CHECK-NEXT: (local.get $z)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (call $foo
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (local.get $4)
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (local.get $5)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: ;;@ file.cpp:20:2
;; CHECK-NEXT: (call $bar
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (local.get $4)
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (local.get $5)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: ;;@ file.cpp:30:3
;; CHECK-NEXT: (call_ref $i32_i32_=>_none
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (local.get $y)
Expand All @@ -287,7 +303,11 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $call_ref-to-select (param $x i32) (param $y i32) (param $z i32) (param $f (ref $i32_i32_=>_none))
;; This call_ref should become an if over two direct calls.
;; This call_ref should become an if over two direct calls. The debuginfo
;; should transfer as well to the two new calls (and some of the new helper
;; code that is generated, but the critical part is the call_ref is being
;; replaced by two calls, which should have the same info).
;;@ file.cpp:20:2
(call_ref $i32_i32_=>_none
(local.get $x)
(local.get $y)
Expand All @@ -299,6 +319,7 @@
)

;; But here one arm is not constant, so we do not optimize.
;;@ file.cpp:30:3
(call_ref $i32_i32_=>_none
(local.get $x)
(local.get $y)
Expand Down