[PATCH] D58260: [INLINER] allow inlining of blockaddresses if sole uses are callbrs
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 17 14:50:57 PDT 2019
nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.
================
Comment at: llvm/include/llvm/Analysis/LazyCallGraph.h:1095
+ return false;
+ }) && Visited.insert(BA->getFunction()).second)
Worklist.push_back(BA->getFunction());
----------------
chandlerc wrote:
> 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?
> we should check the `Visited`
> `if (Visited.count(...))`
Just triple checking that you meant `Visited.insert` (I don't mean to be pedantic, I'm just unclear)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58260/new/
https://reviews.llvm.org/D58260
More information about the llvm-commits
mailing list