[ValueTracking] Use call arguments for nonnull attribute check

[nikic]

Summary

isKnownNonZero() can determine nonnull-ness from a dominating call with a nonnull parameter. Currently, this works by inspecting the attributes on the called function.

This code currently crashes with opaque pointers if the call-site and the called function have a different number of arguments (which no longer requires a bitcast to sit in between). Fix this by directly working with the call arguments instead.

As seen by changes on existing tests, this has the additional benefit that we now take into account attributes that are present only on the callsite, and not on the function declaration.

Unfortunately, this means we need to duplicate the function-level Attribute::hasNonNullAttr() logic into a call-level CallBase::paramIsNonNull() function.

[dexonsmith]

llvm/lib/IR/Instructions.cpp

325

I wonder if (some or most of) this logic could be factored out of here and Argument::hasNonNullAttr() if the common implementation took the Type and an AttributeList as parameters?

[nikic]

llvm/lib/IR/Instructions.cpp

325

I gave this a try: https://gist.github.com/nikic/d1ce3f7a7488be786974e040648a65e0

I'm not sure this really makes things better. Ultimately, the core logic here is simple, and moving it to AttributeSet means that we instead need to duplicate the "call or callee attributes" logic, so it ends up being bulkier overall.

[aeubanks]

are we going to allow mismatched argument types/num arguments for a direct call? it seems bad for usability and seems very easy to mess up frontends, or even simple handwritten IR tests
I'm aware that there are issues with this, it's sort of like [1], but we should probably highlight this as part of the opaque pointer transition

[1]: https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it

[nikic]

In D105313#2868000, @aeubanks wrote:

are we going to allow mismatched argument types/num arguments for a direct call? it seems bad for usability and seems very easy to mess up frontends, or even simple handwritten IR tests
I'm aware that there are issues with this, it's sort of like [1], but we should probably highlight this as part of the opaque pointer transition

[1]: https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it

Good question. I think we do need to allow it for similar reasons. Currently, you can call a function with a different signature by inserting a bitcast between the function type pointers. With opaque pointers we no longer have that bitcast to indicate the signature mismatch.

With that in mind, I think that this patch goes the wrong way about fixing the original problem (though I still think the change makes sense for other reasons, like using call-site attributes). Namely, what getCalledFunction() currently does is simply check whether getCalledOperand() is a function. However, with opaque pointers we should additionally check that the call function type matches the declaration function type. Then call can continue under the reasonable assumption that the call and the declaration match. Effectively this means that we're going to treat a call to a function with a different signature as an indirect, mostly non-analyzable call, the same as we did before. If we don't do this, we break lots of code, especially code handling intrinsics.

This doesn't address the issue of unintentional mistakes. I don't think we can really do anything about that -- it's a general drawback of the opaque pointer approach that you no longer have the sanity check between value types and pointer element types.

[dexonsmith]

In D105313#2868067, @nikic wrote:

Namely, what getCalledFunction() currently does is simply check whether getCalledOperand() is a function.

Duplicating slightly from my comment in https://reviews.llvm.org/D105733, scanning lib/Analysis it looks like many callers to expect it to see through bitcasts, like its header docs suggest it would. Note also that the AbstractCallSite version *does* look through bitcasts:

/// Return the function being called if this is a direct call, otherwise
/// return null (if it's an indirect call).
Function *getCalledFunction() const {
  Value *V = getCalledOperand();
  return V ? dyn_cast<Function>(V->stripPointerCasts()) : nullptr;
}

llvm/include/llvm/IR/InstrTypes.h

uint64_t getDereferenceableOrNullBytes(unsigned i) const {
     return Attrs.getDereferenceableOrNullBytes(i);
   }
 
   /// Return true if the return value is known to be not null.
   /// This may be because it has the nonnull attribute, or because at least
   /// one byte is dereferenceable and the pointer is in addrspace(0).
   bool isReturnNonNull() const;
 
+  /// Return true if this parameter is implied nonnull by attributes. If
+  /// AllowUndefOrPoison is true, then a null parameter is merely poison. If it
+  /// is false, then a null parameter is undefined behavior.
+  bool paramIsNonNull(unsigned ArgNo, bool AllowUndefOrPoison) const;
+
   /// Determine if the return value is marked with NoAlias attribute.
   bool returnDoesNotAlias() const {
     return Attrs.hasAttribute(AttributeList::ReturnIndex, Attribute::NoAlias);
   }
 
   /// If one of the arguments has the 'returned' attribute, returns its
   /// operand value. Otherwise, return nullptr.
   Value *getReturnedArgOperand() const;

llvm/lib/Analysis/ValueTracking.cpp

for (auto *U : V->users()) {
     // Avoid massive lists
     if (NumUsesExplored >= DomConditionsMaxUses)
       break;
     NumUsesExplored++;
 
     // If the value is used as an argument to a call or invoke, then argument
     // attributes may provide an answer about null-ness.
     if (const auto *CB = dyn_cast<CallBase>(U))
-      if (auto *CalledFunc = CB->getCalledFunction())
-        for (const Argument &Arg : CalledFunc->args())
-          if (CB->getArgOperand(Arg.getArgNo()) == V &&
-              Arg.hasNonNullAttr(/* AllowUndefOrPoison */ false) &&
-              DT->dominates(CB, CtxI))
-            return true;
+      for (const Use &Arg : CB->args())
+        if (Arg.get() == V &&
+            CB->paramIsNonNull(CB->getArgOperandNo(&Arg),
+                               /* AllowUndefOrPoison */ false) &&
 
     // If the value is used as a load/store, then the pointer must be non null.
     if (V == getLoadStorePointerOperand(U)) {
       const Instruction *I = cast<Instruction>(U);
       if (!NullPointerIsDefined(I->getFunction(),
                                 V->getType()->getPointerAddressSpace()) &&
           DT->dominates(I, CtxI))
         return true;

llvm/lib/IR/Instructions.cpp

bool CallBase::isReturnNonNull() const {
   if (getDereferenceableBytes(AttributeList::ReturnIndex) > 0 &&
            !NullPointerIsDefined(getCaller(),
                                  getType()->getPointerAddressSpace()))
     return true;
 
   return false;
 }
 
+bool CallBase::paramIsNonNull(unsigned ArgNo, bool AllowUndefOrPoison) const {
+  Type *ArgType = getArgOperand(ArgNo)->getType();
+  if (!ArgType->isPointerTy()) return false;
+  if (paramHasAttr(ArgNo, Attribute::NonNull) &&
+      (AllowUndefOrPoison || paramHasAttr(ArgNo, Attribute::NoUndef)))
+    return true;
+  if (getDereferenceableBytes(ArgNo) > 0 &&
+      !NullPointerIsDefined(getFunction(), ArgType->getPointerAddressSpace()))
+    return true;
+  return false;
+}
+
 Value *CallBase::getReturnedArgOperand() const {
   unsigned Index;
 
   if (Attrs.hasAttrSomewhere(Attribute::Returned, &Index) && Index)
     return getArgOperand(Index - AttributeList::FirstArgIndex);
   if (const Function *F = getCalledFunction())
     if (F->getAttributes().hasAttrSomewhere(Attribute::Returned, &Index) &&
         Index)

llvm/test/Transforms/InstCombine/align-addr.ll

 
 %struct.s = type { i32, i32, i32, i32 }
 
 define void @test3(%struct.s* sret(%struct.s) %a4) {
 ; Check that the alignment is bumped up the alignment of the sret type.
 ; CHECK-LABEL: @test3(
 ; CHECK-NEXT:    [[A4_CAST:%.*]] = bitcast %struct.s* [[A4:%.*]] to i8*
 ; CHECK-NEXT:    call void @llvm.memset.p0i8.i64(i8* noundef nonnull align 4 dereferenceable(16) [[A4_CAST]], i8 0, i64 16, i1 false)
-; CHECK-NEXT:    call void @use(i8* [[A4_CAST]])
+; CHECK-NEXT:    call void @use(i8* nonnull [[A4_CAST]])
 ; CHECK-NEXT:    ret void
 ;
   %a4.cast = bitcast %struct.s* %a4 to i8*
   call void @llvm.memset.p0i8.i64(i8* %a4.cast, i8 0, i64 16, i1 false)
   call void @use(i8* %a4.cast)
   ret void
 }

llvm/test/Transforms/InstCombine/memset_chk-1.ll

 
 ; Test that RAUW in SimplifyLibCalls for __memset_chk generates valid IR
 define i32 @test_rauw(i8* %a, i8* %b, i8** %c) {
 ; CHECK-LABEL: @test_rauw(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[CALL49:%.*]] = call i64 @strlen(i8* noundef nonnull dereferenceable(1) [[A:%.*]])
 ; CHECK-NEXT:    [[ADD180:%.*]] = add i64 [[CALL49]], 1
 ; CHECK-NEXT:    [[YO107:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[B:%.*]], i1 false, i1 false, i1 false)
-; CHECK-NEXT:    [[CALL50:%.*]] = call i8* @__memmove_chk(i8* [[B]], i8* [[A]], i64 [[ADD180]], i64 [[YO107]])
+; CHECK-NEXT:    [[CALL50:%.*]] = call i8* @__memmove_chk(i8* [[B]], i8* nonnull [[A]], i64 [[ADD180]], i64 [[YO107]])
 ; CHECK-NEXT:    [[STRLEN:%.*]] = call i64 @strlen(i8* noundef nonnull dereferenceable(1) [[B]])
 ; CHECK-NEXT:    [[STRCHR1:%.*]] = getelementptr i8, i8* [[B]], i64 [[STRLEN]]
 ; CHECK-NEXT:    [[D:%.*]] = load i8*, i8** [[C:%.*]], align 8
 ; CHECK-NEXT:    [[SUB182:%.*]] = ptrtoint i8* [[D]] to i64
 ; CHECK-NEXT:    [[SUB183:%.*]] = ptrtoint i8* [[B]] to i64
 ; CHECK-NEXT:    [[SUB184:%.*]] = sub i64 [[SUB182]], [[SUB183]]
 ; CHECK-NEXT:    [[ADD52_I_I:%.*]] = add nsw i64 [[SUB184]], 1
 ; CHECK-NEXT:    call void @llvm.memset.p0i8.i64(i8* align 1 [[STRCHR1]], i8 0, i64 [[ADD52_I_I]], i1 false)

 
 declare i8* @__memset_chk(i8*, i32, i64, i64)
 
 ; FIXME: memset(malloc(x), 0, x) -> calloc(1, x)
 
 define float* @pr25892(i64 %size) #0 {
 ; CHECK-LABEL: @pr25892(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[CALL:%.*]] = tail call i8* @malloc(i64 [[SIZE:%.*]]) [[ATTR3:#.*]]
+; CHECK-NEXT:    [[CALL:%.*]] = tail call i8* @malloc(i64 [[SIZE:%.*]]) #[[ATTR3:[0-9]+]]
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i8* [[CALL]], null
 ; CHECK-NEXT:    br i1 [[CMP]], label [[CLEANUP:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.end:
 ; CHECK-NEXT:    [[BC:%.*]] = bitcast i8* [[CALL]] to float*
 ; CHECK-NEXT:    [[CALL2:%.*]] = tail call i64 @llvm.objectsize.i64.p0i8(i8* nonnull [[CALL]], i1 false, i1 false, i1 false)
-; CHECK-NEXT:    [[CALL3:%.*]] = tail call i8* @__memset_chk(i8* nonnull [[CALL]], i32 0, i64 [[SIZE]], i64 [[CALL2]]) [[ATTR3]]
+; CHECK-NEXT:    [[CALL3:%.*]] = tail call i8* @__memset_chk(i8* nonnull [[CALL]], i32 0, i64 [[SIZE]], i64 [[CALL2]]) #[[ATTR3]]
 ; CHECK-NEXT:    br label [[CLEANUP]]
 ; CHECK:       cleanup:
 ; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi float* [ [[BC]], [[IF_END]] ], [ null, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:    ret float* [[RETVAL_0]]
 ;
 entry:
   %call = tail call i8* @malloc(i64 %size) #1
   %cmp = icmp eq i8* %call, null

llvm/test/Transforms/InstCombine/sprintf-1.ll

 ; CHECK-IPRINTF-NEXT:    [[TMP1:%.*]] = ptrtoint i8* [[STPCPY]] to i32
 ; CHECK-IPRINTF-NEXT:    [[TMP2:%.*]] = ptrtoint i8* [[DST]] to i32
 ; CHECK-IPRINTF-NEXT:    [[TMP3:%.*]] = sub i32 [[TMP1]], [[TMP2]]
 ; CHECK-IPRINTF-NEXT:    ret i32 [[TMP3]]
 ;
 ; WIN-LABEL: @test_simplify7(
 ; WIN-NEXT:    [[STRLEN:%.*]] = call i32 @strlen(i8* noundef nonnull dereferenceable(1) [[STR:%.*]])
 ; WIN-NEXT:    [[LENINC:%.*]] = add i32 [[STRLEN]], 1
-; WIN-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 [[DST:%.*]], i8* align 1 [[STR]], i32 [[LENINC]], i1 false)
+; WIN-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 [[DST:%.*]], i8* nonnull align 1 [[STR]], i32 [[LENINC]], i1 false)
 ; WIN-NEXT:    ret i32 [[STRLEN]]
 ;
   %fmt = getelementptr [3 x i8], [3 x i8]* @percent_s, i32 0, i32 0
   %r = call i32 (i8*, i8*, ...) @sprintf(i8* %dst, i8* %fmt, i8* %str)
   ret i32 %r
 }
 
 ; Check sprintf(dst, "%s", str) -> llvm.memcpy(dest, str, strlen(str) + 1, 1).

 ; CHECK-IPRINTF-NEXT:    [[TMP1:%.*]] = ptrtoint i8* [[STPCPY]] to i32
 ; CHECK-IPRINTF-NEXT:    [[TMP2:%.*]] = ptrtoint i8* [[DST]] to i32
 ; CHECK-IPRINTF-NEXT:    [[TMP3:%.*]] = sub i32 [[TMP1]], [[TMP2]]
 ; CHECK-IPRINTF-NEXT:    ret i32 [[TMP3]]
 ;
 ; WIN-LABEL: @test_simplify9(
 ; WIN-NEXT:    [[STRLEN:%.*]] = call i32 @strlen(i8* noundef nonnull dereferenceable(1) [[STR:%.*]])
 ; WIN-NEXT:    [[LENINC:%.*]] = add i32 [[STRLEN]], 1
-; WIN-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 [[DST:%.*]], i8* align 1 [[STR]], i32 [[LENINC]], i1 false)
+; WIN-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 [[DST:%.*]], i8* nonnull align 1 [[STR]], i32 [[LENINC]], i1 false)
 ; WIN-NEXT:    ret i32 [[STRLEN]]
 ;
   %fmt = getelementptr [3 x i8], [3 x i8]* @percent_s, i32 0, i32 0
   %r = call i32 (i8*, i8*, ...) @sprintf(i8* %dst, i8* %fmt, i8* %str)
   ret i32 %r
 }
 
 define void @test_no_simplify1(i8* %dst) {

llvm/test/Transforms/InstCombine/strstr-1.ll

;
   ret i8* %ret
 }
 
 ; Check strstr(str, pat) == str -> strncmp(str, pat, strlen(str)) == 0.
 
 define i1 @test_simplify5(i8* %str, i8* %pat) {
 ; CHECK-LABEL: @test_simplify5(
 ; CHECK-NEXT:    [[STRLEN:%.*]] = call i64 @strlen(i8* noundef nonnull dereferenceable(1) [[PAT:%.*]])
-; CHECK-NEXT:    [[STRNCMP:%.*]] = call i32 @strncmp(i8* [[STR:%.*]], i8* [[PAT]], i64 [[STRLEN]])
+; CHECK-NEXT:    [[STRNCMP:%.*]] = call i32 @strncmp(i8* [[STR:%.*]], i8* nonnull [[PAT]], i64 [[STRLEN]])
 ; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32 [[STRNCMP]], 0
 ; CHECK-NEXT:    ret i1 [[CMP1]]
 ;
   %ret = call i8* @strstr(i8* %str, i8* %pat)
   %cmp = icmp eq i8* %ret, %str
   ret i1 %cmp
 }
 

llvm/test/Transforms/InstSimplify/known-non-zero-opaque-ptrs.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instsimplify -force-opaque-pointers < %s | FileCheck %s
+
+declare void @zero_args()
+declare void @two_args(ptr, ptr)
+
+define i1 @test_zero_args_nonnull(ptr %p) {
+; CHECK-LABEL: @test_zero_args_nonnull(
+; CHECK-NEXT:    call void @zero_args(ptr noundef nonnull [[P:%.*]])
+; CHECK-NEXT:    ret i1 true
+;
+  call void @zero_args(ptr nonnull noundef %p)
+  %c = icmp ne ptr %p, null
+  ret i1 %c
+}
+
+define i1 @test_zero_args_maybe_null(ptr %p) {
+; CHECK-LABEL: @test_zero_args_maybe_null(
+; CHECK-NEXT:    call void @zero_args(ptr [[P:%.*]])
+; CHECK-NEXT:    [[C:%.*]] = icmp ne ptr [[P]], null
+; CHECK-NEXT:    ret i1 [[C]]
+;
+  call void @zero_args(ptr %p)
+  %c = icmp ne ptr %p, null
+  ret i1 %c
+}
+
+define i1 @test_two_args_nonnull(ptr %p) {
+; CHECK-LABEL: @test_two_args_nonnull(
+; CHECK-NEXT:    call void @two_args(ptr noundef nonnull [[P:%.*]])
+; CHECK-NEXT:    ret i1 true
+;
+  call void @two_args(ptr nonnull noundef %p)
+  %c = icmp ne ptr %p, null
+  ret i1 %c
+}
+
+define i1 @test_two_args_maybe_null(ptr %p) {
+; CHECK-LABEL: @test_two_args_maybe_null(
+; CHECK-NEXT:    call void @two_args(ptr [[P:%.*]])
+; CHECK-NEXT:    [[C:%.*]] = icmp ne ptr [[P]], null
+; CHECK-NEXT:    ret i1 [[C]]
+;
+  call void @two_args(ptr %p)
+  %c = icmp ne ptr %p, null
+  ret i1 %c
+}