[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.

LGTM



================
Comment at: llvm/include/llvm/Analysis/LazyCallGraph.h:1095
+          return false;
+        }) && Visited.insert(BA->getFunction()).second)
           Worklist.push_back(BA->getFunction());
----------------
void wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > 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)?
> > > Oh, nvm I see `count` then `insert` later. Ok sorry for the noise.
> > > `      return false; // CHANGE HERE`
> > > If we have a *non* instruction user, I think we should conservatively walk through it. We'd need to recurse trhough it here otherwise.
> > 
> > Doesn't hurt.
> > 
> > > It'd be good to add a test case along thes elines where we round trip through an extra global or something?
> > 
> > Maybe noob question but what's an example of a `Use` of a `Constant` that's not an `Instruction`?
> Values can be used by Global*s and probably other objects.
Yeah. Globals, other constants, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58260