This is an archive of the discontinued LLVM Phabricator instance.

Differential D66161

[SLC] Dereferenceable annonation - handle valid null pointers
ClosedPublic

Authored by xbolva00 on Aug 13 2019, 1:09 PM.

Details

Reviewers
jdoerfert
reames
Commits
rGf94460d4b64c: [SLC] Dereferenceable annonation - handle valid null pointers
rL368884: [SLC] Dereferenceable annonation - handle valid null pointers

Diff Detail

Event Timeline

xbolva00 created this revision.Aug 13 2019, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 1:09 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
xbolva00 added a reviewer: reames.Aug 13 2019, 1:48 PM
xbolva00 planned changes to this revision.Aug 13 2019, 1:57 PM
xbolva00 updated this revision to Diff 214911.Aug 13 2019, 1:59 PM
jdoerfert added a comment.Aug 13 2019, 2:09 PM

I'm fine with this, @reames ?

reames added a comment.Aug 13 2019, 2:36 PM
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 added a comment.Aug 13 2019, 2:52 PM
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 updated this revision to Diff 214935.Aug 13 2019, 3:02 PM

Handle addrspaces.

reames added a comment.Aug 13 2019, 3:02 PM
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 updated this revision to Diff 214938.Aug 13 2019, 3:06 PM

I think this should be safe now.

jdoerfert added a comment.Aug 13 2019, 3:25 PM
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 added a comment.Aug 13 2019, 3:30 PM

nullPointerIsDefined(F, AS) .. cool :)

Test is in extra file due to memcmp redefinition issue.

jdoerfert added inline comments.Aug 13 2019, 3:47 PM
lib/Transforms/Utils/SimplifyLibCalls.cpp
206

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

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

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 updated this revision to Diff 215067.Aug 14 2019, 3:11 AM

Tests.
Addressed review note.

xbolva00 updated this revision to Diff 215071.Aug 14 2019, 3:17 AM
xbolva00 updated this revision to Diff 215073.Aug 14 2019, 3:23 AM
xbolva00 marked an inline comment as done.
xbolva00 marked an inline comment as done.
jdoerfert added a comment.Aug 14 2019, 9:42 AM

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

reames added a comment.Aug 14 2019, 9:46 AM

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 added a comment.EditedAug 14 2019, 9:50 AM
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 accepted this revision.Aug 14 2019, 10:08 AM
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.

This revision is now accepted and ready to land.Aug 14 2019, 10:08 AM
Closed by commit rL368884: [SLC] Dereferenceable annonation - handle valid null pointers (authored by xbolva00). · Explain WhyAug 14 2019, 10:17 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Transforms/
Utils/
12 lines
test/
Transforms/
InstCombine/
13 lines
11 lines

Diff 214938

lib/Transforms/Utils/SimplifyLibCalls.cpp

Show First 20 Lines • Show All 183 Lines • ▼ Show 20 Lines if (CI->getFunction()->hasFnAttribute(Attribute::SanitizeMemory))
return false; return false;
return true; return true;
} }
static void annotateDereferenceableBytes(CallInst *CI, static void annotateDereferenceableBytes(CallInst *CI,
ArrayRef<unsigned> ArgNos, ArrayRef<unsigned> ArgNos,
uint64_t DereferenceableBytes) { uint64_t DereferenceableBytes) {
Function *F = CI->getFunction();
if (!F)
return;
for (unsigned ArgNo : ArgNos) { for (unsigned ArgNo : ArgNos) {
uint64_t DerefBytes = std::max( uint64_t DerefBytes = DereferenceableBytes;
CI->getDereferenceableOrNullBytes(ArgNo + AttributeList::FirstArgIndex), unsigned AS = CI->getArgOperand(ArgNo)->getType()->getPointerAddressSpace();
if (AS == 0 && !F->nullPointerIsDefined())
DerefBytes = std::max(CI->getDereferenceableOrNullBytes(
ArgNo + AttributeList::FirstArgIndex),
DereferenceableBytes); DereferenceableBytes);
if (CI->getDereferenceableBytes(ArgNo + AttributeList::FirstArgIndex) < if (CI->getDereferenceableBytes(ArgNo + AttributeList::FirstArgIndex) <
DerefBytes) { DerefBytes) {
CI->removeParamAttr(ArgNo, Attribute::Dereferenceable); CI->removeParamAttr(ArgNo, Attribute::Dereferenceable);
CI->removeParamAttr(ArgNo, Attribute::DereferenceableOrNull); CI->removeParamAttr(ArgNo, Attribute::DereferenceableOrNull);
jdoerfertUnsubmitted
Done
ReplyInline Actions

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

jdoerfert: Now, one could just keep this annotation if `NullPointerIsDefined(F, AS)` was true.
CI->addParamAttr(ArgNo, Attribute::getWithDereferenceableBytes( CI->addParamAttr(ArgNo, Attribute::getWithDereferenceableBytes(
CI->getContext(), DerefBytes)); CI->getContext(), DerefBytes));
} }
} }
} }
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// String and Memory Library Call Optimizations // String and Memory Library Call Optimizations
▲ Show 20 Lines • Show All 2,994 Lines • Show Last 20 Lines

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) "null-pointer-is-valid"="true" {
jdoerfertUnsubmitted
Done
ReplyInline Actions

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).

jdoerfert: The point was, ` "null-pointer-is-valid"="true" ` should not be needed if the AS is set to (sth…
; CHECK-LABEL: @memcmp_const_size_update_deref8(
; CHECK-NEXT: [[CALL:%.*]] = tail call i32 @memcmp(i8 addrspace(1)* dereferenceable(16) [[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
}

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

Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Lines
; CHECK-LABEL: @memcmp_const_size_update_deref4( ; 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: [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(16) [[D:%.*]], i8* dereferenceable(16) [[S:%.*]], i64 16)
; CHECK-NEXT: ret i32 [[CALL]] ; CHECK-NEXT: ret i32 [[CALL]]
; ;
%call = tail call i32 @memcmp(i8* dereferenceable_or_null(16) %d, i8* %s, i64 16) %call = tail call i32 @memcmp(i8* dereferenceable_or_null(16) %d, i8* %s, i64 16)
ret i32 %call 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-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: [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(40) [[D:%.*]], i8* dereferenceable(16) [[S:%.*]], i64 16)
; CHECK-NEXT: ret i32 [[CALL]] ; CHECK-NEXT: ret i32 [[CALL]]
; ;
%call = tail call i32 @memcmp(i8* dereferenceable_or_null(40) %d, i8* %s, i64 16) %call = tail call i32 @memcmp(i8* dereferenceable_or_null(40) %d, i8* %s, i64 16)
ret i32 %call 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) [[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) { 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-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: [[CALL:%.*]] = tail call i32 @memcmp(i8* dereferenceable(40) [[D:%.*]], i8* dereferenceable(20) [[S:%.*]], i64 16)
; CHECK-NEXT: ret i32 [[CALL]] ; CHECK-NEXT: ret i32 [[CALL]]
; ;
%call = tail call i32 @memcmp(i8* dereferenceable(40) %d, i8* dereferenceable(20) %s, i64 16) %call = tail call i32 @memcmp(i8* dereferenceable(40) %d, i8* dereferenceable(20) %s, i64 16)
ret i32 %call ret i32 %call
} }
▲ Show 20 Lines • Show All 71 Lines • Show Last 20 Lines