Revert "InstSimplify: Require instruction be parented"
authorAlan Zhao <ayzhao@google.com>
Fri, 16 Jun 2023 17:35:52 +0000 (10:35 -0700)
committerAlan Zhao <ayzhao@google.com>
Fri, 16 Jun 2023 17:36:49 +0000 (10:36 -0700)
This reverts commit 1536e299e63d7788f38117b0212ca50eb76d7a3b.

Reason: causes a regression in the inliner (see https://crbug.com/1454531 and https://reviews.llvm.org/rG1536e299e63d7788f38117b0212ca50eb76d7a3b#1217141)

16 files changed:
llvm/docs/ReleaseNotes.rst
llvm/include/llvm/Analysis/InstructionSimplify.h
llvm/include/llvm/IR/BasicBlock.h
llvm/lib/Analysis/InstructionSimplify.cpp
llvm/lib/IR/BasicBlock.cpp
llvm/lib/Transforms/Scalar/JumpThreading.cpp
llvm/lib/Transforms/Utils/CloneFunction.cpp
llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
llvm/test/Transforms/Inline/inline_inv_group.ll
llvm/test/Transforms/Inline/simplify-instruction-computeKnownFPClass-context.ll
llvm/test/Transforms/LoopRotate/pr56260.ll
llvm/test/Transforms/SampleProfile/profile-context-tracker-debug.ll
llvm/test/Transforms/SampleProfile/pseudo-probe-stale-profile-matching.ll
llvm/test/Transforms/SimplifyCFG/pr46638.ll
llvm/unittests/Transforms/Utils/LocalTest.cpp

index 2b1edd5..e66f24b 100644 (file)
@@ -77,9 +77,6 @@ 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
 ------------------------
 
index c3a9c20..0d65041 100644 (file)
 // values. This will prevent other code from seeing the same undef uses and
 // resolving them to different values.
 //
-// They require that all the IR that they encounter be valid and inserted into a
-// parent function.
+// 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).
 //
 // 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
index 19bf954..4e765da 100644 (file)
@@ -251,10 +251,7 @@ public:
 
   /// Unlink this basic block from its current function and insert it into
   /// the function that \p MovePos lives in, right before \p MovePos.
-  inline void moveBefore(BasicBlock *MovePos) {
-    moveBefore(MovePos->getIterator());
-  }
-  void moveBefore(SymbolTableList<BasicBlock>::iterator MovePos);
+  void moveBefore(BasicBlock *MovePos);
 
   /// Unlink this basic block from its current function and insert it
   /// right after \p MovePos in the function \p MovePos lives in.
index 616873f..f125b34 100644 (file)
@@ -6740,7 +6740,6 @@ 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()) {
index 14e1787..5e900e6 100644 (file)
@@ -133,8 +133,9 @@ iplist<BasicBlock>::iterator BasicBlock::eraseFromParent() {
   return getParent()->getBasicBlockList().erase(getIterator());
 }
 
-void BasicBlock::moveBefore(SymbolTableList<BasicBlock>::iterator MovePos) {
-  getParent()->splice(MovePos, getParent(), getIterator());
+void BasicBlock::moveBefore(BasicBlock *MovePos) {
+  MovePos->getParent()->splice(MovePos->getIterator(), getParent(),
+                               getIterator());
 }
 
 void BasicBlock::moveAfter(BasicBlock *MovePos) {
index 5b880f9..b4e86b3 100644 (file)
@@ -2643,7 +2643,6 @@ 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)
@@ -2661,7 +2660,7 @@ bool JumpThreadingPass::duplicateCondBranchOnPHIIntoPred(
             {BB->getModule()->getDataLayout(), TLI, nullptr, nullptr, New})) {
       ValueMapping[&*BI] = IV;
       if (!New->mayHaveSideEffects()) {
-        New->eraseFromParent();
+        New->deleteValue();
         New = nullptr;
       }
     } else {
@@ -2670,6 +2669,7 @@ 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)))
index d552086..272970e 100644 (file)
@@ -470,8 +470,9 @@ void PruningFunctionCloner::CloneBlock(
 
   // Nope, clone it now.
   BasicBlock *NewBB;
-  Twine NewName(BB->hasName() ? Twine(BB->getName()) + NameSuffix : "");
-  BBEntry = NewBB = BasicBlock::Create(BB->getContext(), NewName, NewFunc);
+  BBEntry = NewBB = BasicBlock::Create(BB->getContext());
+  if (BB->hasName())
+    NewBB->setName(BB->getName() + NameSuffix);
 
   // 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
@@ -497,7 +498,6 @@ 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,6 +516,8 @@ 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
@@ -526,7 +528,7 @@ void PruningFunctionCloner::CloneBlock(
 
         if (!NewInst->mayHaveSideEffects()) {
           VMap[&*II] = V;
-          NewInst->eraseFromParent();
+          NewInst->deleteValue();
           continue;
         }
       }
@@ -535,6 +537,7 @@ 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);
@@ -682,8 +685,8 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
     if (!NewBB)
       continue; // Dead block.
 
-    // Move the new block to preserve the order in the original function.
-    NewBB->moveBefore(NewFunc->end());
+    // Add the new block to the new function.
+    NewFunc->insert(NewFunc->end(), NewBB);
 
     // Handle PHI nodes specially, as we have to remove references to dead
     // blocks.
index d81db56..1a9eaf2 100644 (file)
@@ -435,8 +435,6 @@ 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.
@@ -446,7 +444,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->eraseFromParent();
+          C->deleteValue();
           continue;
         }
 
@@ -459,7 +457,7 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
         // in the map.
         InsertNewValueIntoMap(ValueMap, Inst, V);
         if (!C->mayHaveSideEffects()) {
-          C->eraseFromParent();
+          C->deleteValue();
           C = nullptr;
         }
       } else {
@@ -468,6 +466,7 @@ 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);
index a18b45f..684f386 100644 (file)
@@ -3211,9 +3211,6 @@ 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");
 
@@ -3229,8 +3226,7 @@ FoldCondBranchOnValueKnownInPredecessorImpl(BranchInst *BI, DomTreeUpdater *DTU,
         if (!BBI->use_empty())
           TranslateMap[&*BBI] = V;
         if (!N->mayHaveSideEffects()) {
-          N->eraseFromParent(); // Instruction folded away, don't need actual
-                                // inst
+          N->deleteValue(); // Instruction folded away, don't need actual inst
           N = nullptr;
         }
       } else {
@@ -3238,6 +3234,9 @@ FoldCondBranchOnValueKnownInPredecessorImpl(BranchInst *BI, DomTreeUpdater *DTU,
           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)
index f99e90a..0f44f4e 100644 (file)
@@ -14,9 +14,8 @@ define ptr @callee() alwaysinline {
   ret ptr %1
 }
 
-define ptr @caller() null_pointer_is_valid {
-; CHECK-LABEL: define ptr @caller
-; CHECK-SAME: () #[[ATTR1:[0-9]+]] {
+define ptr @caller() {
+; CHECK-LABEL: define ptr @caller() {
 ; CHECK-NEXT:    [[TMP1:%.*]] = call ptr @llvm.strip.invariant.group.p0(ptr null)
 ; CHECK-NEXT:    ret ptr [[TMP1]]
 ;
index 7e9f621..d129f04 100644 (file)
@@ -30,7 +30,8 @@ define i1 @simplify_fcmp_ord_fdiv_caller(double nofpclass(zero nan inf) %i0, dou
 ; CHECK-LABEL: define i1 @simplify_fcmp_ord_fdiv_caller
 ; CHECK-SAME: (double nofpclass(nan inf zero) [[I0:%.*]], double nofpclass(nan inf zero) [[I1:%.*]]) {
 ; CHECK-NEXT:    [[SUB_DOUBLE_SUB_I:%.*]] = fdiv double [[I0]], [[I1]]
-; CHECK-NEXT:    ret i1 true
+; CHECK-NEXT:    [[CMP_I:%.*]] = fcmp ord double [[SUB_DOUBLE_SUB_I]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[CMP_I]]
 ;
   %call = call i1 @simplify_fcmp_ord_fdiv_callee(double %i0, double %i1)
   ret i1 %call
@@ -47,7 +48,8 @@ define i1 @simplify_fcmp_ord_frem_caller(double nofpclass(zero nan inf) %i0, dou
 ; CHECK-LABEL: define i1 @simplify_fcmp_ord_frem_caller
 ; CHECK-SAME: (double nofpclass(nan inf zero) [[I0:%.*]], double nofpclass(nan inf zero) [[I1:%.*]]) {
 ; CHECK-NEXT:    [[SUB_DOUBLE_SUB_I:%.*]] = frem double [[I0]], [[I1]]
-; CHECK-NEXT:    ret i1 true
+; CHECK-NEXT:    [[CMP_I:%.*]] = fcmp ord double [[SUB_DOUBLE_SUB_I]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[CMP_I]]
 ;
   %call = call i1 @simplify_fcmp_ord_frem_callee(double %i0, double %i1)
   ret i1 %call
@@ -64,7 +66,8 @@ define i1 @simplify_fcmp_ord_fmul_caller(double nofpclass(zero nan inf) %i0, dou
 ; CHECK-LABEL: define i1 @simplify_fcmp_ord_fmul_caller
 ; CHECK-SAME: (double nofpclass(nan inf zero) [[I0:%.*]], double nofpclass(nan inf zero) [[I1:%.*]]) {
 ; CHECK-NEXT:    [[SUB_DOUBLE_SUB_I:%.*]] = fmul double [[I0]], [[I1]]
-; CHECK-NEXT:    ret i1 true
+; CHECK-NEXT:    [[CMP_I:%.*]] = fcmp ord double [[SUB_DOUBLE_SUB_I]], 0.000000e+00
+; CHECK-NEXT:    ret i1 [[CMP_I]]
 ;
   %call = call i1 @simplify_fcmp_ord_fmul_callee(double %i0, double %i1)
   ret i1 %call
index 70b68e7..41c8b6a 100644 (file)
@@ -14,17 +14,18 @@ define void @main() {
 ; CHECK:       L0.preheader:
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 0, 0
 ; CHECK-NEXT:    [[INC:%.*]] = zext i1 [[CMP]] to i32
-; CHECK-NEXT:    [[TOBOOL3_NOT1:%.*]] = icmp eq i32 [[INC]], 0
-; CHECK-NEXT:    br i1 [[TOBOOL3_NOT1]], label [[L0_PREHEADER_LOOPEXIT]], label [[L1_PREHEADER_LR_PH:%.*]]
+; CHECK-NEXT:    [[SPEC_SELECT1:%.*]] = add nsw i32 0, [[INC]]
+; CHECK-NEXT:    [[TOBOOL3_NOT2:%.*]] = icmp eq i32 [[SPEC_SELECT1]], 0
+; CHECK-NEXT:    br i1 [[TOBOOL3_NOT2]], label [[L0_PREHEADER_LOOPEXIT]], label [[L1_PREHEADER_LR_PH:%.*]]
 ; CHECK:       L1.preheader.lr.ph:
 ; CHECK-NEXT:    br label [[L1_PREHEADER:%.*]]
 ; CHECK:       L1.preheader:
-; CHECK-NEXT:    [[SPEC_SELECT3:%.*]] = phi i32 [ [[INC]], [[L1_PREHEADER_LR_PH]] ], [ [[SPEC_SELECT:%.*]], [[L0_LATCH:%.*]] ]
-; CHECK-NEXT:    [[K_02:%.*]] = phi i32 [ 0, [[L1_PREHEADER_LR_PH]] ], [ [[SPEC_SELECT3]], [[L0_LATCH]] ]
-; CHECK-NEXT:    [[TOBOOL8_NOT:%.*]] = icmp eq i32 [[K_02]], 0
+; CHECK-NEXT:    [[SPEC_SELECT4:%.*]] = phi i32 [ [[SPEC_SELECT1]], [[L1_PREHEADER_LR_PH]] ], [ [[SPEC_SELECT:%.*]], [[L0_LATCH:%.*]] ]
+; CHECK-NEXT:    [[K_03:%.*]] = phi i32 [ 0, [[L1_PREHEADER_LR_PH]] ], [ [[SPEC_SELECT4]], [[L0_LATCH]] ]
+; CHECK-NEXT:    [[TOBOOL8_NOT:%.*]] = icmp eq i32 [[K_03]], 0
 ; CHECK-NEXT:    br label [[L0_LATCH]]
 ; CHECK:       L0.latch:
-; CHECK-NEXT:    [[SPEC_SELECT]] = add nsw i32 [[SPEC_SELECT3]], [[INC]]
+; CHECK-NEXT:    [[SPEC_SELECT]] = add nsw i32 [[SPEC_SELECT4]], [[INC]]
 ; CHECK-NEXT:    [[TOBOOL3_NOT:%.*]] = icmp eq i32 [[SPEC_SELECT]], 0
 ; CHECK-NEXT:    br i1 [[TOBOOL3_NOT]], label [[L0_L0_PREHEADER_LOOPEXIT_CRIT_EDGE:%.*]], label [[L1_PREHEADER]]
 ;
index bb0abb1..bd670a1 100644 (file)
@@ -27,7 +27,7 @@
 ; INLINE-ALL-NEXT: Getting callee context for instr:   %call.i = tail call i32 @_Z8funcLeafi
 ; INLINE-ALL-NEXT:   Callee context found: main:3 @ _Z5funcAi:1 @ _Z8funcLeafi
 ; INLINE-ALL-NEXT: Marking context profile as inlined: main:3 @ _Z5funcAi:1 @ _Z8funcLeafi
-; INLINE-ALL-NEXT: Getting callee context for instr:   %call.i2 = tail call i32 @_Z3fibi
+; INLINE-ALL-NEXT: Getting callee context for instr:   %call.i1 = tail call i32 @_Z3fibi
 ; INLINE-ALL-NEXT: Getting callee context for instr:   %call5.i = tail call i32 @_Z3fibi
 ; INLINE-ALL-DAG:  Getting base profile for function: _Z5funcAi
 ; INLINE-ALL-DAG-NEXT:   Merging context profile into base profile: _Z5funcAi
index 89477ea..34a494f 100644 (file)
@@ -80,9 +80,9 @@
 ; CHECK:    5:  call void @llvm.pseudoprobe(i64 -2624081020897602054, i64 5, i32 0, i64 -1), !dbg ![[#]] - weight: 0 - factor: 1.00)
 ; CHECK:    1:  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 1, i32 0, i64 -1), !dbg ![[#]] - weight: 112 - factor: 1.00)
 ; CHECK:    2:  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 2, i32 0, i64 -1), !dbg ![[#]] - weight: 101 - factor: 1.00)
-; CHECK:    5:  %call.i8 = call i32 @bar(i32 noundef %1), !dbg ![[#]] - weight: 101 - factor: 1.00)
+; CHECK:    5:  %call.i3 = call i32 @bar(i32 noundef %1), !dbg ![[#]] - weight: 101 - factor: 1.00)
 ; CHECK:    3:  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 3, i32 0, i64 -1), !dbg ![[#]] - weight: 13 - factor: 1.00)
-; CHECK:    6:  %call1.i5 = call i32 @bar(i32 noundef %add.i4), !dbg ![[#]] - weight: 13 - factor: 1.00)
+; CHECK:    6:  %call1.i6 = call i32 @bar(i32 noundef %add.i5), !dbg ![[#]] - weight: 13 - factor: 1.00)
 ; CHECK:    4:  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 4, i32 0, i64 -1), !dbg ![[#]] - weight: 112 - factor: 1.00)
 ; CHECK:    14:  %call2 = call i32 @bar(i32 noundef %3), !dbg ![[#]] - weight: 124 - factor: 1.00)
 ; CHECK:    8:  call void @llvm.pseudoprobe(i64 -2624081020897602054, i64 8, i32 0, i64 -1), !dbg ![[#]] - weight: 0 - factor: 1.00)
index 2c8ad62..8e72249 100644 (file)
@@ -15,7 +15,9 @@ define void @pr46638(i1 %c, i32 %x) {
 ; CHECK:       common.ret:
 ; CHECK-NEXT:    ret void
 ; CHECK:       true2.critedge:
-; CHECK-NEXT:    call void @dummy(i32 0)
+; CHECK-NEXT:    [[CMP2_C:%.*]] = icmp sgt i32 [[X]], 0
+; CHECK-NEXT:    [[EXT_C:%.*]] = zext i1 [[CMP2_C]] to i32
+; CHECK-NEXT:    call void @dummy(i32 [[EXT_C]])
 ; CHECK-NEXT:    call void @dummy(i32 2)
 ; CHECK-NEXT:    br label [[COMMON_RET]]
 ;
index 2c59322..537abd9 100644 (file)
@@ -588,6 +588,21 @@ TEST_F(SalvageDebugInfoTest, RecursiveBlockSimplification) {
   verifyDebugValuesAreSalvaged();
 }
 
+TEST(Local, SimplifyVScaleWithRange) {
+  LLVMContext C;
+  Module M("Module", C);
+
+  IntegerType *Ty = Type::getInt32Ty(C);
+  Function *VScale = Intrinsic::getDeclaration(&M, Intrinsic::vscale, {Ty});
+  auto *CI = CallInst::Create(VScale, {}, "vscale");
+
+  // Test that simplifyCall won't try to query it's parent function for
+  // vscale_range attributes in order to simplify llvm.vscale -> constant.
+  EXPECT_EQ(simplifyCall(CI, VScale, {}, SimplifyQuery(M.getDataLayout())),
+            nullptr);
+  delete CI;
+}
+
 TEST(Local, wouldInstructionBeTriviallyDead) {
   LLVMContext Ctx;
   std::unique_ptr<Module> M = parseIR(Ctx,