[SLC] Dereferenceable annonation - handle valid null pointers

[jdoerfert]

I'm fine with this, @reames ?

[reames]

In D66161#1627898, @jdoerfert wrote:

I'm fine with this, @reames ?

Definitely not. I've never seen that utility on Function, but whatever it's supposed to do, it completely ignores address spaces (our major source of defined nulls) while doing it.

A much simpler surgical fix would be to replace the use of getDereferenceableOrNullBytes with getDereferenceableBytes. This would eliminate the "or null concern" from the merge logic.

Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

[jdoerfert]

In D66161#1627965, @reames wrote:

In D66161#1627898, @jdoerfert wrote:

I'm fine with this, @reames ?

Definitely not. I've never seen that utility on Function, but whatever it's supposed to do, it completely ignores address spaces (our major source of defined nulls) while doing it.

The utility function can take a address space and it is used in other places too. To give it the address spaces of the argument pointers would be an option, but ...

A much simpler surgical fix would be to replace the use of getDereferenceableOrNullBytes with getDereferenceableBytes. This would eliminate the "or null concern" from the merge logic.

Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

... we can, as @reames noted, not change the _or_null part at all and only work on the deref(X) parts.

[xbolva00]

Handle addrspaces.

[reames]

In D66161#1627965, @reames wrote:

...
Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

Hey, read back over this and realized my tone rather snappy here. Sorry about that, was annoyed at something else at the time I wrote this and it came through more than I realized. Sorry.

[xbolva00]

I think this should be safe now.

[jdoerfert]

In D66161#1628077, @xbolva00 wrote:

I think this should be safe now.

if (!F->nullPointerIsDefined(AS)) is what I was expecting.

And a test is missing.

[xbolva00]

nullPointerIsDefined(F, AS) .. cool :)

Test is in extra file due to memcmp redefinition issue.

[jdoerfert]

lib/Transforms/Utils/SimplifyLibCalls.cpp

206 ↗

(On Diff #214938)

Now, one could just keep this annotation if NullPointerIsDefined(F, AS) was true.

test/Transforms/InstCombine/mem-deref-bytes-addrspaces.ll

6 ↗

(On Diff #214938)

The point was, "null-pointer-is-valid"="true" should not be needed if the AS is set to (sth not known to have an invalid null).

[xbolva00]

Tests.
Addressed review note.

[jdoerfert]

I like it. We don't loose information and it should be always correct. @reames, still concerned?

[reames]

I have no problem with this patch (i.e. it can land if Johannes approves)

A conceptual point worth mentioning is that seems to essentially be inferring non-null for the intrinsics in question. We have a nonnull attribute on params and nonnull + deref_or_null should already be converted to deref. (If not, we should fix that.) It might be worth thinking about how to leverage the more generic code.

[xbolva00]

In D66161#1629559, @reames wrote:

I have no problem with this patch (i.e. it can land if Johannes approves)

A conceptual point worth mentioning is that seems to essentially be inferring non-null for the intrinsics in question. We have a nonnull attribute on params and nonnull + deref_or_null should already be converted to deref. (If not, we should fix that.) It might be worth thinking about how to leverage the more generic code.

Next step (https://reviews.llvm.org/D53342) :) As noted in D53342 we need to be careful about adding nonnull.

[jdoerfert]

In D66161#1629559, @reames wrote:

I have no problem with this patch (i.e. it can land if Johannes approves)

LGTM.

A conceptual point worth mentioning is that seems to essentially be inferring non-null for the intrinsics in question. We have a nonnull attribute on params and nonnull + deref_or_null should already be converted to deref. (If not, we should fix that.) It might be worth thinking about how to leverage the more generic code.

The Attributor does this already but there are a few points where we need to improve it.
I think these patches should encode "known" information and the Attributor (run early) will combine whatever it can.

llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp

if (CI->getFunction()->hasFnAttribute(Attribute::SanitizeMemory))
     return false;
 
   return true;
 }
 
 static void annotateDereferenceableBytes(CallInst *CI,
                                          ArrayRef<unsigned> ArgNos,
                                          uint64_t DereferenceableBytes) {
+  const Function *F = CI->getFunction();
+  if (!F)
+    return;
   for (unsigned ArgNo : ArgNos) {
-    uint64_t DerefBytes = std::max(
-        CI->getDereferenceableOrNullBytes(ArgNo + AttributeList::FirstArgIndex),
+    uint64_t DerefBytes = DereferenceableBytes;
+    unsigned AS = CI->getArgOperand(ArgNo)->getType()->getPointerAddressSpace();
+    if (!llvm::NullPointerIsDefined(F, AS))
+      DerefBytes = std::max(CI->getDereferenceableOrNullBytes(
+                                ArgNo + AttributeList::FirstArgIndex),
                             DereferenceableBytes);
 
     if (CI->getDereferenceableBytes(ArgNo + AttributeList::FirstArgIndex) <
         DerefBytes) {
       CI->removeParamAttr(ArgNo, Attribute::Dereferenceable);
+      if (!llvm::NullPointerIsDefined(F, AS))
         CI->removeParamAttr(ArgNo, Attribute::DereferenceableOrNull);
       CI->addParamAttr(ArgNo, Attribute::getWithDereferenceableBytes(
                                   CI->getContext(), DerefBytes));
     }
   }
 }
 
 //===----------------------------------------------------------------------===//
 // String and Memory Library Call Optimizations

llvm/trunk/test/Transforms/InstCombine/mem-deref-bytes-addrspaces.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -instcombine  -S < %s | FileCheck %s
+
+declare i32 @memcmp(i8 addrspace(1)* nocapture, i8* nocapture, i64)
+
+define i32 @memcmp_const_size_update_deref8(i8 addrspace(1)* nocapture readonly %d, i8* nocapture readonly %s) {
+; CHECK-LABEL: @memcmp_const_size_update_deref8(
+; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @memcmp(i8 addrspace(1)* dereferenceable(16) dereferenceable_or_null(40) [[D:%.*]], i8* dereferenceable(16) [[S:%.*]], i64 16)
+; CHECK-NEXT:    ret i32 [[CALL]]
+;
+  %call = tail call i32 @memcmp(i8 addrspace(1)* dereferenceable_or_null(40) %d, i8* %s, i64 16)
+  ret i32 %call
+}

llvm/trunk/test/Transforms/InstCombine/mem-deref-bytes.ll

 ; CHECK-LABEL: @memcmp_const_size_update_deref4(
 ; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(16) [[D:%.*]], i8* dereferenceable(16) [[S:%.*]], i64 16)
 ; CHECK-NEXT:    ret i32 [[CALL]]
 ;
   %call = tail call i32 @memcmp(i8* dereferenceable_or_null(16) %d, i8* %s, i64 16)
   ret i32 %call
 }
 
-define i32 @memcmp_const_size_update_deref5(i8* nocapture readonly %d, i8* nocapture readonly %s) {
+define i32 @memcmp_const_size_update_deref5(i8* nocapture readonly %d, i8* nocapture readonly %s) "null-pointer-is-valid"="false" {
 ; CHECK-LABEL: @memcmp_const_size_update_deref5(
 ; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(40) [[D:%.*]], i8* dereferenceable(16) [[S:%.*]], i64 16)
 ; CHECK-NEXT:    ret i32 [[CALL]]
 ;
   %call = tail call i32 @memcmp(i8* dereferenceable_or_null(40) %d, i8* %s, i64 16)
   ret i32 %call
 }
 
+define i32 @memcmp_const_size_update_deref6(i8* nocapture readonly %d, i8* nocapture readonly %s) "null-pointer-is-valid"="true" {
+; CHECK-LABEL: @memcmp_const_size_update_deref6(
+; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(16) dereferenceable_or_null(40) [[D:%.*]], i8* dereferenceable(16) [[S:%.*]], i64 16)
+; CHECK-NEXT:    ret i32 [[CALL]]
+;
+  %call = tail call i32 @memcmp(i8* dereferenceable_or_null(40) %d, i8* %s, i64 16)
+  ret i32 %call
+}
+
 define i32 @memcmp_const_size_no_update_deref(i8* nocapture readonly %d, i8* nocapture readonly %s) {
 ; CHECK-LABEL: @memcmp_const_size_no_update_deref(
 ; CHECK-NEXT:    [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(40) [[D:%.*]], i8* dereferenceable(20) [[S:%.*]], i64 16)
 ; CHECK-NEXT:    ret i32 [[CALL]]
 ;
   %call = tail call i32 @memcmp(i8* dereferenceable(40) %d, i8* dereferenceable(20) %s, i64 16)
   ret i32 %call
 }