[PATCH] D151254: InstSimplify: Require instruction be parented

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/docs/ReleaseNotes.rst:74
+* InstructionSimplify APIs now require instructions be inserted into a
+   parent function.
+
----------------
too much indent


================
Comment at: llvm/include/llvm/Analysis/InstructionSimplify.h:22
 //
-// These routines are designed to tolerate moderately incomplete IR, such as
-// instructions that are not connected to basic blocks yet. However, they do
-// require that all the IR that they encounter be valid. In particular, they
-// require that all non-constant values be defined in the same function, and the
-// same call context of that function (and not split between caller and callee
-// contexts of a directly recursive call, for example).
+// They do require that all the IR that they encounter be valid and inserted
+// into a parent function.
----------------
drop the "do", doesn't really make sense anymore.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:689
-    // Add the new block to the new function.
-    NewFunc->insert(NewFunc->end(), NewBB);
-
----------------
Can we do something like `NewBB->moveBefore(NewFunc->end())` here?

The nice thing about the current implementation is that it will use the same block order as in the original function, which seems like a nice property to preserve.

I also expect that changing the order will result in massive codegen diffs, for a patch that should otherwise be close to NFC.


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

https://reviews.llvm.org/D151254