[PATCH] D76248: Fix a bug in the inliner that causes subsequent double inlining

[Hongtao Yu via Phabricator via llvm-commits]

hoyFB marked an inline comment as done.
hoyFB added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:361
+          // away later on.
+          if (!dyn_cast<CallBase>(NewInst) || !dyn_cast<CallBase>(V)) {
+            VMap[&*II] = V;
----------------
davidxl wrote:
> hoyFB wrote:
> > davidxl wrote:
> > > hoyFB wrote:
> > > > davidxl wrote:
> > > > > Is the first check needed (!dyn_cast<CallBase>(NewInst)) ?
> > > > It's needed when the inlined instruction is not a call but promoted to a call during the simplification. For example, some code on arm32/arm64 target could be promoted to a call or an intrinsic call. In this case we'd like to keep this simplification since the it will not cause double inlining given that the original instruction is not a call.
> > > Ok. Perhaps add a comment in the code?
> > Sounds good.
> It should be ok to do the simplification if V has no side effects .
A pure function call could have no side effect and may still cause double inlining. As long as V is a call, it could be shared by NewInst and another call site. Note that an irregular call graph processing order may not guarantee a pure function call like V be fully inlined before processing NewInst.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76248