Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
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
27 changes: 11 additions & 16 deletions src/ir/debug.h → src/ir/debuginfo.cpp
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,19 +14,16 @@
* 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,
Expression* copy,
Function* originFunc,
Function* copyFunc) {
void copyDebugInfoBetweenFunctions(Expression* origin,
Expression* copy,
Function* originFunc,
Function* copyFunc) {
if (originFunc->debugLocations.empty()) {
return; // No debug info to copy
}
Expand All @@ -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
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 expression and another that it replaces, copy the debug info from
// the latter to the former. Note that 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 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.

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 copyDebugInfoBetweenFunctions(Expression* origin,
Expression* copy,
Function* originFunc,
Function* copyFunc);
} // namespace wasm::debuginfo

#endif // wasm_ir_debuginfo_h
5 changes: 3 additions & 2 deletions src/ir/module-utils.cpp
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,8 @@ 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::copyDebugInfoBetweenFunctions(
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
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::copyDebugInfoBetweenFunctions(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
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::copyDebugInfoToReplacement(ret, curr, &func);
return ret;
};
auto* ifTrueCall = makeCall(ifTrueCallInfo);
auto* ifFalseCall = makeCall(ifFalseCallInfo);
Expand Down
33 changes: 3 additions & 30 deletions src/wasm-traversal.h
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::copyDebugInfoToReplacement(
expression, getCurrent(), currFunction);
}
return *replacep = expression;
}
Expand Down
25 changes: 23 additions & 2 deletions test/lit/passes/optimize-instructions-call_ref.wast
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

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!

;; 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