[InstCombine] Avoid instruction sinking if not profitable for other passes

[xbolva00]

Summary

Part of PR39119

[xbolva00]

• Fixed formatting

[xbolva00]

simplify test cases

[xbolva00]

• Check if null pointer is defined

[xbolva00]

@efriedma, Is this acceptable or not? In the future, the instruction sinking would be moved out of instcombine, but the call inst sinking problem needs to be solved anyway. I don't think we should disable call inst sinking totally.

[lebedev.ri]

I see that there were some perf numbers in https://bugs.llvm.org/show_bug.cgi?id=39119#c11,
but those are global, not for this patch. Anything for this change specifically?

test/Transforms/InstCombine/no-sink-nonnull-args.ll

2

Please precommit the test. It isn't obvious what the diff changes here.
No existing tests change?

[xbolva00]

I will precommit them if folks say this is fine direction to go.

If we sink the call instructions, Florian's new SCCP patches related to null checks optimizations are unable to benefit from the dominating call with nonnull args.

Florian disabled sinking to see the potential of his new SCCP optimizations, but we need a stronger solution (to not regress in other areas).

[lebedev.ri]

In D53098#1262408, @xbolva00 wrote:

I will precommit them if folks say this is fine direction to go.

If no existing test is affected by this, you most likely want the tests anyway.

If we sink the call instructions, Florian's new SCCP patches related to null checks optimizations are unable to benefit from the dominating call with nonnull args.

Florian disabled sinking to see the potential of his new SCCP optimizations, but we need a stronger solution (to not regress in other areas).

[xbolva00]

Ok, the problem I have is not related with sinking. Closing this revision.

I want to mark call sites of C functions with nonnull attr. like "strlen (nonnull s)". But since EnableNonnullArgPropagation is disabled, the attrs are not promoted to parent function arguments. And that's the reason why my motivating example from PR39119 with strcmp is not optimized :(

I still think if we safely mark call sites (or unmark attrs of function if we don't know exact size; for C functions with size arg), enabling "NonnullArgPropagation" should not harm. Eli?

lib/Transforms/InstCombine/InstructionCombining.cpp

if (LI.isCleanup() != CleanupFlag) {
     assert(!CleanupFlag && "Adding a cleanup, not removing one?!");
     LI.setCleanup(CleanupFlag);
     return &LI;
   }
 
   return nullptr;
 }
 
+static bool isProfitableToSink(CallInst &CI) {
+  // If the function is called some nonnull argument, we should not sink it.
+  // Later passes can be more powerful (e.g. optimize null checks)
+  for (auto &Arg : CI.getCalledFunction()->args()) {
+    auto *FArg = dyn_cast<Argument>(CI.getArgOperand(Arg.getArgNo()));
+    if (FArg && FArg->hasNonNullAttr()) {
+      return false;
+    }
+  }
+  return true;
+}
+
 /// Try to move the specified instruction from its current block into the
 /// beginning of DestBlock, which can only happen if it's safe to move the
 /// instruction past all of the instructions between it and the end of its
 /// block.
 static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
   assert(I->hasOneUse() && "Invariants didn't hold!");
   BasicBlock *SrcBlock = I->getParent();
 
   // Cannot move control-flow-involving, volatile loads, vaarg, etc.
   if (isa<PHINode>(I) || I->isEHPad() || I->mayHaveSideEffects() ||
       I->isTerminator())
     return false;
 
   // Do not sink alloca instructions out of the entry block.
-  if (isa<AllocaInst>(I) && I->getParent() ==
-        &DestBlock->getParent()->getEntryBlock())
+  if (isa<AllocaInst>(I) &&
+      I->getParent() == &DestBlock->getParent()->getEntryBlock())
     return false;
 
   // Do not sink into catchswitch blocks.
   if (isa<CatchSwitchInst>(DestBlock->getTerminator()))
     return false;
 
   // Do not sink convergent call instructions.
   if (auto *CI = dyn_cast<CallInst>(I)) {
-    if (CI->isConvergent())
+    if (CI->isConvergent() || !isProfitableToSink(*CI))
       return false;
   }
-  // We can only sink load instructions if there is nothing between the load and
+  // We can only sink load instructions if there is nothing between the load
+  // and
   // the end of block that could change the value.
   if (I->mayReadFromMemory()) {
     for (BasicBlock::iterator Scan = I->getIterator(),
                               E = I->getParent()->end();
          Scan != E; ++Scan)
       if (Scan->mayWriteToMemory())
         return false;
   }
   BasicBlock::iterator InsertPos = DestBlock->getFirstInsertionPt();
   I->moveBefore(&*InsertPos);
   ++NumSunkInst;
 
-  // Also sink all related debug uses from the source basic block. Otherwise we
+  // Also sink all related debug uses from the source basic block. Otherwise
+  // we
   // get debug use before the def.
   SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
   findDbgUsers(DbgUsers, I);
   for (auto *DII : DbgUsers) {
     if (DII->getParent() == SrcBlock) {
       DII->moveBefore(&*InsertPos);
       LLVM_DEBUG(dbgs() << "SINK: " << *DII << '\n');
     }
   }
   return true;
   }
 
   bool InstCombiner::run() {
     while (!Worklist.isEmpty()) {
       Instruction *I = Worklist.RemoveOne();
-    if (I == nullptr) continue;  // skip null values.
+      if (I == nullptr)
+        continue; // skip null values.
 
       // Check to see if we can DCE the instruction.
       if (isInstructionTriviallyDead(I, &TLI)) {
         LLVM_DEBUG(dbgs() << "IC: DCE: " << *I << '\n');
         eraseInstFromFunction(*I);
         ++NumDeadInst;
         MadeIRChange = true;
         continue;
       }
 
       if (!DebugCounter::shouldExecute(VisitCounter))
         continue;
 
       // Instruction isn't dead, see if we can constant propagate it.
       if (!I->use_empty() &&
           (I->getNumOperands() == 0 || isa<Constant>(I->getOperand(0)))) {
         if (Constant *C = ConstantFoldInstruction(I, DL, &TLI)) {
-        LLVM_DEBUG(dbgs() << "IC: ConstFold to: " << *C << " from: " << *I
-                          << '\n');
+          LLVM_DEBUG(dbgs()
+                     << "IC: ConstFold to: " << *C << " from: " << *I << '\n');
 
           // Add operands to the worklist.
           replaceInstUsesWith(*I, C);
           ++NumConstProp;
           if (isInstructionTriviallyDead(I, &TLI))
             eraseInstFromFunction(*I);
           MadeIRChange = true;
           continue;
         }
       }
 
-    // In general, it is possible for computeKnownBits to determine all bits in
+      // In general, it is possible for computeKnownBits to determine all bits
+      // in
       // a value even when the operands are not all constants.
       Type *Ty = I->getType();
       if (ExpensiveCombines && !I->use_empty() && Ty->isIntOrIntVectorTy()) {
-      KnownBits Known = computeKnownBits(I, /*Depth*/0, I);
+        KnownBits Known = computeKnownBits(I, /*Depth*/ 0, I);
         if (Known.isConstant()) {
           Constant *C = ConstantInt::get(Ty, Known.getConstant());
           LLVM_DEBUG(dbgs() << "IC: ConstFold (all bits known) to: " << *C
                             << " from: " << *I << '\n');
 
           // Add operands to the worklist.
           replaceInstUsesWith(*I, C);
           ++NumConstProp;
           if (isInstructionTriviallyDead(I, &TLI))
             eraseInstFromFunction(*I);
           MadeIRChange = true;
           continue;
         }
       }
 
-    // See if we can trivially sink this instruction to a successor basic block.
+      // See if we can trivially sink this instruction to a successor basic
+      // block.
       if (I->hasOneUse()) {
         BasicBlock *BB = I->getParent();
         Instruction *UserInst = cast<Instruction>(*I->user_begin());
         BasicBlock *UserParent;
 
         // Get the block the use occurs in.
         if (PHINode *PN = dyn_cast<PHINode>(UserInst))
           UserParent = PN->getIncomingBlock(*I->use_begin());
         else
           UserParent = UserInst->getParent();
 
         if (UserParent != BB) {
           bool UserIsSuccessor = false;
           // See if the user is one of our successors.
-        for (succ_iterator SI = succ_begin(BB), E = succ_end(BB); SI != E; ++SI)
+          for (succ_iterator SI = succ_begin(BB), E = succ_end(BB); SI != E;
+               ++SI)
             if (*SI == UserParent) {
               UserIsSuccessor = true;
               break;
             }
 
-        // If the user is one of our immediate successors, and if that successor
+          // If the user is one of our immediate successors, and if that
+          // successor
           // only has us as a predecessors (we'd have to split the critical edge
           // otherwise), we can keep going.
           if (UserIsSuccessor && UserParent->getUniquePredecessor()) {
             // Okay, the CFG is simple enough, try to sink this instruction.
             if (TryToSinkInstruction(I, UserParent)) {
               LLVM_DEBUG(dbgs() << "IC: Sink: " << *I << '\n');
               MadeIRChange = true;
               // We'll add uses of the sunk instruction below, but since sinking
               // can expose opportunities for it's *operands* add them to the
               // worklist
               for (Use &U : I->operands())
                 if (Instruction *OpI = dyn_cast<Instruction>(U.get()))
                   Worklist.Add(OpI);
             }
           }
         }
       }
 
       // Now that we have an instruction, try combining it to simplify it.
       Builder.SetInsertPoint(I);
       Builder.SetCurrentDebugLocation(I->getDebugLoc());
 
 #ifndef NDEBUG
     std::string OrigI;
 #endif
     LLVM_DEBUG(raw_string_ostream SS(OrigI); I->print(SS); OrigI = SS.str(););
     LLVM_DEBUG(dbgs() << "IC: Visiting: " << OrigI << '\n');
 
     if (Instruction *Result = visit(*I)) {

test/Transforms/InstCombine/no-sink-nonnull-args.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+define i32 @no_sink_nonnull(i8* nonnull readonly %s) {
+; CHECK-LABEL: @no_sink_nonnull(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @foo(i8* nonnull [[S:%.*]])
+; CHECK-NEXT:    br i1 false, label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    unreachable
+; CHECK:       if.end:
+; CHECK-NEXT:    ret i32 [[CALL]]
+;
+entry:
+  %call = tail call i32 @foo(i8* nonnull %s)
+  %tobool = icmp eq i8* %s, null
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:
+  tail call void @abort()
+  unreachable
+
+if.end:
+  ret i32 %call
+}
+
+define i32 @sink(i8* readonly %s) {
+; CHECK-LABEL: @sink(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp eq i8* [[S:%.*]], null
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    tail call void @abort()
+; CHECK-NEXT:    unreachable
+; CHECK:       if.end:
+; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @foo(i8* nonnull [[S]])
+; CHECK-NEXT:    ret i32 [[CALL]]
+;
+entry:
+  %call = tail call i32 @foo(i8* %s)
+  %tobool = icmp eq i8* %s, null
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:
+  tail call void @abort()
+  unreachable
+
+if.end:
+  ret i32 %call
+}
+
+; Function Attrs: argmemonly nounwind readonly
+declare i32 @foo(i8* nocapture) local_unnamed_addr #1
+declare void @abort()
+
+attributes #1 = { argmemonly nounwind readonly "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }