Skip to content
Open
Show file tree
Hide file tree
Changes from all 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: 0 additions & 1 deletion src/passes/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ set(passes_SOURCES
Precompute.cpp
Print.cpp
PrintBoundary.cpp
PrintCallGraph.cpp
PrintFeatures.cpp
PrintFunctionMap.cpp
RoundTrip.cpp
Expand Down
84 changes: 84 additions & 0 deletions src/passes/GlobalEffects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
#include "support/utilities.h"
#include "wasm.h"

#include <iostream>
#include <sstream>

namespace wasm {

namespace {
Expand Down Expand Up @@ -369,8 +372,89 @@ struct DiscardGlobalEffects : public Pass {
}
};

struct PrintCallGraph : public Pass {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait why is PrintCallGraph now inside GlobalEffects..?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I mention this in the PR description, I chose to keep it here since the logic is somewhat specific to GlobalEffects. e.g. it's not obvious in general that a call graph would have edges from HeapType -> Function to indicate that the function has a given type. I could extract out the call graph logic into a shared header and then use that in both GlobalEffects and a call graph printing pass, but it seems a bit weird since I'm not sure if the call graph generation logic would be applicable anywhere else. I'm ok with going either way though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. Well, my concern is that it seems very surprising to see a general Print* method inside GlobalEffects. PrintGlobalEffects might make sense, but this is a lot more general?

What would go into the shared header? It seems like the existing PrintCallGraph code can stay where it is, and just add edges from heap types to functions - is that more than a few lines of code?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For my purposes, I'd like to look at the exact same call graph that GlobalEffects is already generating, so I think I'd move this code from GlobalEffects (plus the filtering logic from #8644 once it's in) into a header and then re-use that between GlobalEffects and the print call graph pass. Does it make sense to extract it out or should I keep it here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, the current PrintCallGraph pass already scans calls, and I don't think it needs effects - what would it use from the code that you linked to?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, the highlighted lines were a bit imprecise. I'd want to extract out buildCallGraph, but that requires us to track the indirect + direct calls from each function which we currently do in analyzeFuncs (we pass funcInfos to generateCallGraph). The current GlobalEffects also computes effects for each function at the same time that it notes the indirect + direct calls. If we extracted it out, I guess we'd just do a second pass over the functions to do that? Or maybe I'd introduce some way of composing ParallelFunctionAnalysis and running them together in one pass.

Basically if I extract out buildCallGraph it seems like we'd have to compute the call graph and the effects in two separate passes over the module's functions, is that ok?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, now I see, so it would track the indirect calls too.

Yes, extracting something to do that makes sense.

In fact, how about extending the existing CallGraphPropertyAnalysis from module-utils.h, with indirect call tracking? Then all existing users would benefit automatically from a more precise analysis (at least in closed world). And then both PrintCallGraph and GlobalEffects can use that utility.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Interesting, that looks reasonable. Let me see if I can add it there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's not obvious in general that a call graph would have edges from HeapType -> Function to indicate that the function has a given type.

I agree with @kripken that pulling this logic out into a general location sounds fine. Even if HeapType -> Function edges are not necessarily the most intuitive things, I think they are a sufficiently general and expressive way to represent this information that we may as well adopt them everywhere we care about call graphs.

bool modifiesBinaryenIR() override { return false; }

void run(Module* module) override {
std::map<Function*, FuncInfo> funcInfos =
analyzeFuncs(*module, getPassOptions());

auto callGraph =
buildCallGraph(*module, funcInfos, getPassOptions().worldMode);

printCallGraph(callGraph, std::cout);
}

private:
std::string nodeToString(const CallGraphNode& node) {
return std::visit(
overloaded{[](Function* func) { return func->name.toString(); },
[](HeapType type) {
std::stringstream ss;
ss << type;
return ss.str();
}},
node);
}

void printCallGraph(const CallGraph& callGraph, std::ostream& o) {
std::map<std::string, std::string> nodeTypes;
std::map<std::string, std::map<std::string, std::string>> sortedGraph;

auto getNodeType = [](const CallGraphNode& node) {
return std::visit(overloaded{[](Function*) { return "function"; },
[](HeapType) { return "type"; }},
node);
};

for (const auto& [caller, callees] : callGraph) {
std::string callerName = nodeToString(caller);
nodeTypes[callerName] = getNodeType(caller);

for (const auto& callee : callees) {
std::string calleeName = nodeToString(callee);
nodeTypes[calleeName] = getNodeType(callee);

std::string style = std::visit(
overloaded{
[](Function*, Function*) {
return " [style=\"solid\", color=\"black\", kind=\"direct\"]";
},
[](Function*, HeapType) {
return " [style=\"dotted\", color=\"black\", kind=\"indirect\"]";
},
[](HeapType, HeapType) {
return " [style=\"solid\", color=\"blue\", kind=\"subtyping\"]";
},
[](HeapType, Function*) {
return " [style=\"dashed\", color=\"green\", "
"kind=\"implementation\"]";
}},
caller,
callee);

sortedGraph[callerName][calleeName] = style;
}
}

o << "digraph CallGraph {\n";
for (const auto& [nodeName, nodeType] : nodeTypes) {
o << " \"" << nodeName << "\" [kind=\"" << nodeType << "\"];\n";
}
for (const auto& [callerName, callees] : sortedGraph) {
for (const auto& [calleeName, style] : callees) {
o << " \"" << callerName << "\" -> \"" << calleeName << "\"" << style
<< ";\n";
}
}
o << "}\n";
}
};

} // namespace

Pass* createPrintCallGraphPass() { return new PrintCallGraph(); }

Pass* createGenerateGlobalEffectsPass() { return new GenerateGlobalEffects(); }

Pass* createDiscardGlobalEffectsPass() { return new DiscardGlobalEffects(); }
Expand Down
110 changes: 0 additions & 110 deletions src/passes/PrintCallGraph.cpp

This file was deleted.

88 changes: 88 additions & 0 deletions test/lit/passes/print-call-graph.wast
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
;; RUN: foreach %s %t wasm-opt -all --print-call-graph | filecheck %s
;; RUN: foreach %s %t wasm-opt -all --print-call-graph --closed-world | filecheck %s --check-prefix CLOSED

(module
(type $indirect-type (func (param i32)))

(table 1 1 funcref)
(elem $indirect-1 (i32.const 0))

(func $indirect-1 (type $indirect-type) (param i32)
)

(func $indirect-2 (type $indirect-type) (param i32)
)

(func $A (param $ref (ref $indirect-type))
(call_ref $indirect-type (i32.const 42) (local.get $ref))
(call $B (local.get $ref))
)

(func $B (param $ref (ref $indirect-type))
(call_indirect (type $indirect-type) (i32.const 0) (i32.const 0))
)
)

;; CHECK: digraph CallGraph {
;; CHECK-NEXT: "A" [kind="function"];
;; CHECK-NEXT: "B" [kind="function"];
;; CHECK-NEXT: "indirect-1" [kind="function"];
;; CHECK-NEXT: "indirect-2" [kind="function"];
;; CHECK-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"];
;; CHECK-NEXT: }

;; CLOSED: digraph CallGraph {
;; CLOSED-NEXT: "(type $func.0 (func (param (ref $func.1))))" [kind="type"];
;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" [kind="type"];
;; CLOSED-NEXT: "A" [kind="function"];
;; CLOSED-NEXT: "B" [kind="function"];
;; CLOSED-NEXT: "indirect-1" [kind="function"];
;; CLOSED-NEXT: "indirect-2" [kind="function"];
;; CLOSED-NEXT: "(type $func.0 (func (param (ref $func.1))))" -> "A" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "(type $func.0 (func (param (ref $func.1))))" -> "B" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" -> "indirect-1" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" -> "indirect-2" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "A" -> "(type $func.0 (func (param i32)))" [style="dotted", color="black", kind="indirect"];
;; CLOSED-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"];
;; CLOSED-NEXT: "B" -> "(type $func.0 (func (param i32)))" [style="dotted", color="black", kind="indirect"];
;; CLOSED-NEXT: }

;; Test cycles
(module
(type $indirect-type (func (param i32)))
(table 1 1 funcref)

(func $A
(call $B)
)
(func $B
(call $A)
)

(func $indirect-cycle (type $indirect-type) (param $x i32)
(call_indirect (type $indirect-type) (local.get $x) (i32.const 0))
)
(elem $indirect-cycle (i32.const 0))
)

;; CHECK: digraph CallGraph {
;; CHECK-NEXT: "A" [kind="function"];
;; CHECK-NEXT: "B" [kind="function"];
;; CHECK-NEXT: "indirect-cycle" [kind="function"];
;; CHECK-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"];
;; CHECK-NEXT: "B" -> "A" [style="solid", color="black", kind="direct"];
;; CHECK-NEXT: }

;; CLOSED: digraph CallGraph {
;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" [kind="type"];
;; CLOSED-NEXT: "(type $func.0 (func))" [kind="type"];
;; CLOSED-NEXT: "A" [kind="function"];
;; CLOSED-NEXT: "B" [kind="function"];
;; CLOSED-NEXT: "indirect-cycle" [kind="function"];
;; CLOSED-NEXT: "(type $func.0 (func (param i32)))" -> "indirect-cycle" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "(type $func.0 (func))" -> "A" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "(type $func.0 (func))" -> "B" [style="dashed", color="green", kind="implementation"];
;; CLOSED-NEXT: "A" -> "B" [style="solid", color="black", kind="direct"];
;; CLOSED-NEXT: "B" -> "A" [style="solid", color="black", kind="direct"];
;; CLOSED-NEXT: "indirect-cycle" -> "(type $func.0 (func (param i32)))" [style="dotted", color="black", kind="indirect"];
;; CLOSED-NEXT: }
Loading
Loading