[PATCH] D33850: Inlining: Don't re-map simplified cloned instructions.

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

From the summary:

> When simplifying an instruction that has been re-mapped, it should never

simplify to an instruction in the original function.
This looks reasonable to me

> In the edge case

where we are inlining a function into itself, the existing code led to
incorrect behavior.

The  SimpleInliner doesn't do recursive inlining AFAIK. I thought there was a check in InlineFunction as well to disallow this, but I don't see it. So I guess a recursive call may get inlined. It is not obvious to me what the incorrect behavior is in that case. Could you please explain what causes the incorrect behavior?

>   Replace the incorrect code with an assert verifying

that we never expect simplification to produce an instruction in the old
function, unless the functions are the same.

The asserts look reasonable to me. I don't understand the intention of the original code though.



================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:457
   SmallVector<const PHINode*, 16> PHIToResolve;
-  for (const BasicBlock &BI : *OldFunc) {
+  for (const BasicBlock &BI : make_range(OldFunc->begin(), OldFunc->end())) {
     Value *V = VMap.lookup(&BI);
----------------
Unrelated change?


Repository:
  rL LLVM

https://reviews.llvm.org/D33850