[PATCH] D58260: [INLINER] allow inlining of blockaddresses if sole uses are callbrs

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

One edge case left I think, but otherwise this LGTM. Happy for you to submit with the suggested change and a test case.



================
Comment at: llvm/include/llvm/Analysis/LazyCallGraph.h:1095
+          return false;
+        }) && Visited.insert(BA->getFunction()).second)
           Worklist.push_back(BA->getFunction());
----------------
My clang-format spidey sense tingles here...


Also, come to think of it, we should check the `Visited` set first even though it means hashing it twice. And I think we can use early exit to make this more clear.

```
if (Visited.count(...))
  continue;
if (all_of(..., [](...) {
      if (Instruction *I = ...)
        return I->getFunction() == BA->getFunction();
      return false; // CHANGE HERE
    }))
  continue;

Visited.inesrt(...);
Worklist.push_back(...);
```

I think this will be a lot easier to understand.

I've also suggested a change in behavior. If we have a *non* instruction user, I think we should conservatively walk through it. We'd need to recurse trhough it here otherwise.

It'd be good to add a test case along thes elines where we round trip through an extra global or something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58260/new/

https://reviews.llvm.org/D58260