InstSimplify: Require instruction be parented

Unlike every other analysis and transform, simplifyInstruction
permitted operating on instructions which are not inserted
into a function. This created an edge case no other code needs
to really worry about, and limited transforms in cases that
can make use of the context function. Only the inliner and a handful
of other utilities were making use of this, so just fix up these
edge cases. Results in some IR ordering differences since
cloned blocks are inserted eagerly now. Plus some additional
simplifications trigger (e.g. some add 0s now folded out that
previously didn't).

Author: arsenm

llvm/docs/ReleaseNotes.rst

@@ -75,6 +75,9 @@ Changes to LLVM infrastructure
   legacy inliner pass. Backend stack coloring should handle cases alloca
   merging initially set out to handle.
 
+* InstructionSimplify APIs now require instructions be inserted into a
+  parent function.
+
 Changes to building LLVM
 ------------------------
 

llvm/include/llvm/Analysis/InstructionSimplify.h

@@ -19,12 +19,8 @@
 // values. This will prevent other code from seeing the same undef uses and
 // resolving them to different values.
 //
-// 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 require that all the IR that they encounter be valid and inserted into a
+// parent function.
 //
 // Additionally, these routines can't simplify to the instructions that are not
 // def-reachable, meaning we can't just scan the basic block for instructions

llvm/include/llvm/IR/BasicBlock.h

@@ -251,7 +251,10 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
 
   /// Unlink this basic block from its current function and insert it into
   /// the function that \p MovePos lives in, right before \p MovePos.
-  void moveBefore(BasicBlock *MovePos);
+  inline void moveBefore(BasicBlock *MovePos) {
+    moveBefore(MovePos->getIterator());
+  }
+  void moveBefore(SymbolTableList<BasicBlock>::iterator MovePos);
 
   /// Unlink this basic block from its current function and insert it
   /// right after \p MovePos in the function \p MovePos lives in.

llvm/lib/Analysis/InstructionSimplify.cpp

@@ -6748,6 +6748,7 @@ static Value *simplifyInstructionWithOperands(Instruction *I,
                                               ArrayRef<Value *> NewOps,
                                               const SimplifyQuery &SQ,
                                               unsigned MaxRecurse) {
+  assert(I->getFunction() && "instruction should be inserted in a function");
   const SimplifyQuery Q = SQ.CxtI ? SQ : SQ.getWithInstruction(I);
 
   switch (I->getOpcode()) {

llvm/lib/IR/BasicBlock.cpp

@@ -133,9 +133,8 @@ iplist<BasicBlock>::iterator BasicBlock::eraseFromParent() {
   return getParent()->getBasicBlockList().erase(getIterator());
 }
 
-void BasicBlock::moveBefore(BasicBlock *MovePos) {
-  MovePos->getParent()->splice(MovePos->getIterator(), getParent(),
-                               getIterator());
+void BasicBlock::moveBefore(SymbolTableList<BasicBlock>::iterator MovePos) {
+  getParent()->splice(MovePos, getParent(), getIterator());
 }
 
 void BasicBlock::moveAfter(BasicBlock *MovePos) {

llvm/lib/Transforms/Scalar/JumpThreading.cpp

@@ -2643,6 +2643,7 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
   // mapping and using it to remap operands in the cloned instructions.
   for (; BI != BB->end(); ++BI) {
     Instruction *New = BI->clone();
+    New->insertInto(PredBB, OldPredBranch->getIterator());
 
     // Remap operands to patch up intra-block references.
     for (unsigned i = 0, e = New->getNumOperands(); i != e; ++i)
@@ -2660,7 +2661,7 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
             {BB->getModule()->getDataLayout(), TLI, nullptr, nullptr, New})) {
       ValueMapping[&*BI] = IV;
       if (!New->mayHaveSideEffects()) {
-        New->deleteValue();
+        New->eraseFromParent();
         New = nullptr;
       }
     } else {
@@ -2669,7 +2670,6 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
     if (New) {
       // Otherwise, insert the new instruction into the block.
       New->setName(BI->getName());
-      New->insertInto(PredBB, OldPredBranch->getIterator());
       // Update Dominance from simplified New instruction operands.
       for (unsigned i = 0, e = New->getNumOperands(); i != e; ++i)
         if (BasicBlock *SuccBB = dyn_cast<BasicBlock>(New->getOperand(i)))

llvm/lib/Transforms/Utils/CloneFunction.cpp

@@ -470,9 +470,8 @@ void PruningFunctionCloner::CloneBlock(
 
   // Nope, clone it now.
   BasicBlock *NewBB;
-  BBEntry = NewBB = BasicBlock::Create(BB->getContext());
-  if (BB->hasName())
-    NewBB->setName(BB->getName() + NameSuffix);
+  Twine NewName(BB->hasName() ? Twine(BB->getName()) + NameSuffix : "");
+  BBEntry = NewBB = BasicBlock::Create(BB->getContext(), NewName, NewFunc);
 
   // It is only legal to clone a function if a block address within that
   // function is never referenced outside of the function.  Given that, we
@@ -498,6 +497,7 @@ void PruningFunctionCloner::CloneBlock(
        ++II) {
 
     Instruction *NewInst = cloneInstruction(II);
+    NewInst->insertInto(NewBB, NewBB->end());
 
     if (HostFuncIsStrictFP) {
       // All function calls in the inlined function must get 'strictfp'
@@ -516,8 +516,6 @@ void PruningFunctionCloner::CloneBlock(
       // If we can simplify this instruction to some other value, simply add
       // a mapping to that value rather than inserting a new instruction into
       // the basic block.
-      //
-      // FIXME: simplifyInstruction should know the context of the new function.
       if (Value *V =
               simplifyInstruction(NewInst, BB->getModule()->getDataLayout())) {
         // On the off-chance that this simplifies to an instruction in the old
@@ -528,7 +526,7 @@ void PruningFunctionCloner::CloneBlock(
 
         if (!NewInst->mayHaveSideEffects()) {
           VMap[&*II] = V;
-          NewInst->deleteValue();
+          NewInst->eraseFromParent();
           continue;
         }
       }
@@ -537,7 +535,6 @@ void PruningFunctionCloner::CloneBlock(
     if (II->hasName())
       NewInst->setName(II->getName() + NameSuffix);
     VMap[&*II] = NewInst; // Add instruction map to value.
-    NewInst->insertInto(NewBB, NewBB->end());
     if (isa<CallInst>(II) && !II->isDebugOrPseudoInst()) {
       hasCalls = true;
       hasMemProfMetadata |= II->hasMetadata(LLVMContext::MD_memprof);
@@ -685,8 +682,8 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
     if (!NewBB)
       continue; // Dead block.
 
-    // Add the new block to the new function.
-    NewFunc->insert(NewFunc->end(), NewBB);
+    // Move the new block to preserve the order in the original function.
+    NewBB->moveBefore(NewFunc->end());
 
     // Handle PHI nodes specially, as we have to remove references to dead
     // blocks.

llvm/lib/Transforms/Utils/LoopRotationUtils.cpp

@@ -435,6 +435,8 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
 
       // Otherwise, create a duplicate of the instruction.
       Instruction *C = Inst->clone();
+      C->insertBefore(LoopEntryBranch);
+
       ++NumInstrsDuplicated;
 
       // Eagerly remap the operands of the instruction.
@@ -444,7 +446,7 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
       // Avoid inserting the same intrinsic twice.
       if (auto *DII = dyn_cast<DbgVariableIntrinsic>(C))
         if (DbgIntrinsics.count(makeHash(DII))) {
-          C->deleteValue();
+          C->eraseFromParent();
           continue;
         }
 
@@ -457,7 +459,7 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
         // in the map.
         InsertNewValueIntoMap(ValueMap, Inst, V);
         if (!C->mayHaveSideEffects()) {
-          C->deleteValue();
+          C->eraseFromParent();
           C = nullptr;
         }
       } else {
@@ -466,7 +468,6 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
       if (C) {
         // Otherwise, stick the new instruction into the new block!
         C->setName(Inst->getName());
-        C->insertBefore(LoopEntryBranch);
 
         if (auto *II = dyn_cast<AssumeInst>(C))
           AC->registerAssumption(II);

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

@@ -3211,6 +3211,9 @@ FoldCondBranchOnValueKnownInPredecessorImpl(BranchInst *BI, DomTreeUpdater *DTU,
       }
       // Clone the instruction.
       Instruction *N = BBI->clone();
+      // Insert the new instruction into its new home.
+      N->insertInto(EdgeBB, InsertPt);
+
       if (BBI->hasName())
         N->setName(BBI->getName() + ".c");
 
@@ -3226,17 +3229,15 @@ FoldCondBranchOnValueKnownInPredecessorImpl(BranchInst *BI, DomTreeUpdater *DTU,
         if (!BBI->use_empty())
           TranslateMap[&*BBI] = V;
         if (!N->mayHaveSideEffects()) {
-          N->deleteValue(); // Instruction folded away, don't need actual inst
+          N->eraseFromParent(); // Instruction folded away, don't need actual
+                                // inst
           N = nullptr;
         }
       } else {
         if (!BBI->use_empty())
           TranslateMap[&*BBI] = N;
       }
       if (N) {
-        // Insert the new instruction into its new home.
-        N->insertInto(EdgeBB, InsertPt);
-
         // Register the new instruction with the assumption cache if necessary.
         if (auto *Assume = dyn_cast<AssumeInst>(N))
           if (AC)

llvm/test/Transforms/Inline/inline_inv_group.ll

@@ -14,8 +14,9 @@ define ptr @callee() alwaysinline {
   ret ptr %1
 }
 
-define ptr @caller() {
-; CHECK-LABEL: define ptr @caller() {
+define ptr @caller() null_pointer_is_valid {
+; CHECK-LABEL: define ptr @caller
+; CHECK-SAME: () #[[ATTR1:[0-9]+]] {
 ; CHECK-NEXT:    [[TMP1:%.*]] = call ptr @llvm.strip.invariant.group.p0(ptr null)
 ; CHECK-NEXT:    ret ptr [[TMP1]]
 ;