[PATCH] D31722: [llvm-extract] Add option for recursive extraction

[Davide Italiano via Phabricator via llvm-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

Please add a basic test and a test with mutually recursive calls. The basic logic seems fine to me, some nits (sorry for being pedantic).



================
Comment at: tools/llvm-extract/llvm-extract.cpp:236
+    std::vector<llvm::Function *> Workqueue;
+    for (auto *GV : GVs) {
+      if (Function *F = dyn_cast<Function>(GV)) {
----------------
explicit type when non-obvious.


================
Comment at: tools/llvm-extract/llvm-extract.cpp:244
+      Workqueue.pop_back();
+      ExitOnErr(F->materialize());
+      for (auto &BB : *F) {
----------------
Can we have a slightly better diagnostic here? Something like Cannot materialize function `F` + `F->getName()`.


================
Comment at: tools/llvm-extract/llvm-extract.cpp:245
+      ExitOnErr(F->materialize());
+      for (auto &BB : *F) {
+        for (auto &I : BB) {
----------------
I generally prefer early continue to avoid many levels of nesting (there are a few in this case), so I'd rewrite the loop as:
```
for (...) {
  auto *CI = dyn_cast<>();
  if (!CI)
    continue;
  [...]
}
```

but I don't feel strong about it, so, up to you.


================
Comment at: tools/llvm-extract/llvm-extract.cpp:247
+        for (auto &I : BB) {
+          if (CallInst *CI = dyn_cast<CallInst>(&I)) {
+            if (Function *CF = CI->getCalledFunction()) {
----------------
and probably `auto` when the type is obvious`.


https://reviews.llvm.org/D31722