SCCP: Don't assert on constantexpr casts of function uses

[arsenm]

Summary

This includes 2 different, related fixes:

1. Fix asserting on direct assume-like intrinsic uses of a function
   address

2. Fix asserting on constant expression casts used by assume-like
   intrinsics.

By default hasAddressTaken permits assume-like intrinsic uses, which
ignores assume-like calls and pointer casts of the address used by
assume-like calls.

Fixes issue 59602, but there are additional issues I encountered when
debugging this. For instance, the original failing bitcast expression
was really unused. Clang tentatively created it for the function type,
but was unnecessary after applyGlobalValReplacements. That did not
clean up the now dead ConstantExpr which hung around oun the user
list, so this assert only reproduced when running clang from the
original testcase, and didn't just running opt -passes=ipsccp. I don't
know who is responsible for cleaning up unused ConstantExprs, but I've
run into similar issues several times recently.

Additionally, I found a few assertions with llvm.ssa.copy with
functions and casts of functions as the argument.

Another issue theoretically exists if hasAddressTaken chooses to
respect nocapture when passed function addresses. The search here
would need to do additional work to look at the users of the constant
cast to see if any call sites need returned to be stripped.

[nikic]

LG

I don't know who is responsible for cleaning up unused ConstantExprs, but I've run into similar issues several times recently.

I guess in this particular case SCCP could clean them up (it already does so for GlobalVariables, see the removeDeadConstantUsers() call). It's also possible to ignore them via hasZeroLiveUses().

llvm/lib/Transforms/IPO/SCCP.cpp

357

If I understood things correctly, I don't think so: We only look through casts for assume-like intrinsics, and for those it's not a call use, so there's no attributes to drop.

[fhahn]

Thanks for the patch!

llvm/lib/Transforms/IPO/SCCP.cpp

87

It would probably be good to add a comment here explaining why we skip assume-like intrinsics here.

355–358

IIUC we only want to adjust the attributes for direct calls, and skip assume-like calls, so something like below?

if (CB->getCalledFunction() != F) {
   assert(cast<IntrinsicInst>(CB)->isAssumeLikeIntrinsic());
   continue;
 }

[fhahn]

It would also be good to include Fixes #59602 in the description to have GitHub auto-close the issue.

[arsenm]

llvm/lib/Transforms/IPO/SCCP.cpp

357

I think if hasAddressTaken looked at nocapture attributes, we could reach here

[arsenm]

llvm/lib/Transforms/IPO/SCCP.cpp

355–358

In principle yes, but ConstantExpr makes everything more difficult as always. The case that failed here is actually the constantexpr cast, and then you need to consider the users.

[arsenm]

llvm/lib/Transforms/IPO/SCCP.cpp

355–358

Well almost, it's really the existing isa<BlockAddress> or a dead constantexpr here. IntrinsicInst satisfies CallBase

[arsenm]

Add assert

[arsenm]

ad6b5762b64f6706cb4d1bcad383c373736b5af6

llvm/lib/Transforms/IPO/SCCP.cpp

assert(
                if (!isa<CallBase>(U))
                  return true;
                if (U->getType()->isStructTy()) {
                  return all_of(Solver.getStructLatticeValueFor(U),
                                [](const ValueLatticeElement &LV) {
                                  return !isOverdefined(LV);
                                });
                }
+
+               // We don't consider assume-like intrinsics to be actual address
+               // captures.
+               if (auto *II = dyn_cast<IntrinsicInst>(U)) {
+                 if (II->isAssumeLikeIntrinsic())
+                   return true;
+               }
+
                return !isOverdefined(Solver.getLatticeValueFor(U));
              }) &&
       "We can only zap functions where all live users have a concrete value");
 
   for (BasicBlock &BB : F) {
     if (CallInst *CI = BB.getTerminatingMustTailCall()) {
       LLVM_DEBUG(dbgs() << "Can't zap return of the block due to present "
                         << "musttail call : " << *CI << "\n");
static bool runIPSCCP(
   }
 
   // Remove the returned attribute for zapped functions and the
   // corresponding call sites.
   for (Function *F : FuncZappedReturn) {
     for (Argument &A : F->args())
       F->removeParamAttr(A.getArgNo(), Attribute::Returned);
     for (Use &U : F->uses()) {
-      // Skip over blockaddr users.
-      if (isa<BlockAddress>(U.getUser()))
+      CallBase *CB = dyn_cast<CallBase>(U.getUser());
+      if (!CB) {
+        assert(isa<BlockAddress>(U.getUser()) ||
+               (isa<Constant>(U.getUser()) &&
+                all_of(U.getUser()->users(), [](const User *UserUser) {
+                  return cast<IntrinsicInst>(UserUser)->isAssumeLikeIntrinsic();
+                })));
         continue;
-      CallBase *CB = cast<CallBase>(U.getUser());
+      }
+
+      // FIXME: Could this be a ConstantExpr cast user with a use in a call site
+      // that needs to be handled?
       for (Use &Arg : CB->args())
         CB->removeParamAttr(CB->getArgOperandNo(&Arg), Attribute::Returned);
     }
   }
 
   // If we inferred constant or undef values for globals variables, we can
   // delete the global and any stores that remain to it.
   for (const auto &I : make_early_inc_range(Solver.getTrackedGlobals())) {

llvm/test/Transforms/SCCP/issue59602-assume-like-call-users.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature
+; RUN: opt -S -passes=ipsccp < %s | FileCheck %s
+
+@extern = global i32 0
+
+define i32 @call_assume_self_user() {
+; CHECK-LABEL: define {{[^@]+}}@call_assume_self_user() {
+; CHECK-NEXT:    [[CALL:%.*]] = call i32 @assume_self_user()
+; CHECK-NEXT:    ret i32 0
+;
+  %call = call i32 @assume_self_user()
+  ret i32 0
+}
+
+; Function address direct user is assume-like intrinsic.
+;
+; Make sure this doesn't hit "We can only zap functions where all live
+; users have a concrete value"
+define internal i32 @assume_self_user() {
+; CHECK-LABEL: define {{[^@]+}}@assume_self_user() {
+; CHECK-NEXT:    [[OBJSIZE:%.*]] = call i32 @llvm.objectsize.i32.p0(ptr @assume_self_user, i1 false, i1 false, i1 false)
+; CHECK-NEXT:    store i32 [[OBJSIZE]], ptr @extern, align 4
+; CHECK-NEXT:    ret i32 undef
+;
+  %objsize = call i32 @llvm.objectsize.i32.p0(ptr @assume_self_user, i1 false, i1 false, i1 false)
+  store i32 %objsize, ptr @extern
+  ret i32 0
+}
+
+
+; The objectsize call is an "assume like intrinsic" that can be
+; ignored for address taken purposes. The user of @constexpr_self_user
+; is a constant expression cast, so that the use call site does not
+; directly appear as a user.
+
+define i32 @callsite_with_returned() {
+; CHECK-LABEL: define {{[^@]+}}@callsite_with_returned() {
+; CHECK-NEXT:    [[CALL:%.*]] = call addrspace(1) i32 @constexpr_self_user(i32 123)
+; CHECK-NEXT:    ret i32 0
+;
+  %call = call addrspace(1) i32 @constexpr_self_user(i32 returned 123)
+  ret i32 0
+}
+
+; Has constexpr cast user in an assume-like intrinsic. Make sure we
+; don't assert on a cast<CallBase> for the user, and still remove
+; returned from the caller.
+define internal i32 @constexpr_self_user(i32 %arg0) addrspace(1) {
+; CHECK-LABEL: define {{[^@]+}}@constexpr_self_user
+; CHECK-SAME: (i32 [[ARG0:%.*]]) addrspace(1) {
+; CHECK-NEXT:    [[OBJSIZE:%.*]] = call i32 @llvm.objectsize.i32.p0(ptr addrspacecast (ptr addrspace(1) @constexpr_self_user to ptr), i1 false, i1 false, i1 false)
+; CHECK-NEXT:    store i32 [[OBJSIZE]], ptr @extern, align 4
+; CHECK-NEXT:    ret i32 undef
+;
+  %objsize = call i32 @llvm.objectsize.i32.p0(ptr addrspacecast (ptr addrspace(1) @constexpr_self_user to ptr), i1 false, i1 false, i1 false)
+  store i32 %objsize, ptr @extern
+  ret i32 %arg0
+}
+
+declare i32 @llvm.objectsize.i32.p0(ptr, i1 immarg, i1 immarg, i1 immarg) #0
+
+attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }