[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

1

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 with a nonnull argument, we should not sink the
+  // call. Later passes can be more powerful (e.g. optimize null checks)
+  Function *F = CI.getFunction();
+  if (!F || F->nullPointerIsDefined())
+    return true;
+
+  if (Function *CalledFunc = CI.getCalledFunction()) {
+    for (auto &Arg : CalledFunc->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())
     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
   // 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)

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
+}
+
+define i32 @sink_null_defined(i8* nonnull readonly %s) #1 {
+; CHECK-LABEL: @sink_null_defined(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br i1 false, label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then:
+; 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* 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
+}
+
+declare i32 @foo(i8* nocapture) #0
+declare void @abort()
+
+attributes #0 = { argmemonly nounwind readonly }
+attributes #1 = { "null-pointer-is-valid"="true" }