This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] -fno-delete-null-pointer-checks: change dereferenceable to dereferenceable_or_null
ClosedPublic

Authored by MaskRay on Nov 29 2020, 10:56 PM.

Details

Summary

After D17993, with -fno-delete-null-pointer-checks we add the dereferenceable attribute to the this pointer.

We have observed that one internal target which worked before fails even with -fno-delete-null-pointer-checks.
Switching to dereferenceable_or_null fixes the problem.

dereferenceable currently does not always respect NullPointerIsValid and may
imply nonnull and lead to aggressive optimization. The optimization may be
related to CallBase::isReturnNonNull, Argument::hasNonNullAttr, or
Value::getPointerDereferenceableBytes. See D66664 and D66618 for some discussions.

Diff Detail

Event Timeline

MaskRay created this revision.Nov 29 2020, 10:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2020, 10:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay requested review of this revision.Nov 29 2020, 10:56 PM
MaskRay edited the summary of this revision. (Show Details)Nov 29 2020, 11:27 PM
MaskRay edited the summary of this revision. (Show Details)Nov 29 2020, 11:36 PM

This was discussed on the original patch (https://reviews.llvm.org/D17993#inline-830995) and @jdoerfert argued that dereferenceable is correct even in NullPointerIsValid mode. Does this indicate a bug in the middle-end? If so, we should add a FIXME here to remove the workaround once the underlying bug is fixed.

bkramer accepted this revision.Nov 30 2020, 4:35 AM

While it would be nice for dereferenceable to not imply nonnull, the implementation currently assumes it does and will speculate loads based on that. See llvm::Value::getPointerDereferenceableBytes and its users.

This revision is now accepted and ready to land.Nov 30 2020, 4:35 AM

While it would be nice for dereferenceable to not imply nonnull, the implementation currently assumes it does and will speculate loads based on that. See llvm::Value::getPointerDereferenceableBytes and its users.

LLVM-Core unfortunately conflated null as 0x000000 and as "invalid pointer"/"non-dereferencable". This patch just works around that. While this is fine for now, a FIXME should be placed and LLVM-Core needs fixing.

I briefly scanned my history and found D66664 which is relevant and gives some context and my attempt to fix this last year.

MaskRay updated this revision to Diff 308419.Nov 30 2020, 10:26 AM
MaskRay edited the summary of this revision. (Show Details)

Add FIXME

MaskRay added inline comments.Nov 30 2020, 10:28 AM
clang/lib/CodeGen/CGCall.cpp
2176

@rsmith @jdoerfert Am I right about the FIXME issue here?

Should I link to D66618 instead?

rsmith accepted this revision.Nov 30 2020, 12:33 PM

Looks good. There are a few other places where we unconditionally insert dereferenceable attributes (for reference parameters / return types, and for C99's T [static] array parameters). We should presumably apply the same workaround everywhere we add dereferenceable.

clang/lib/CodeGen/CGCall.cpp
2178
MaskRay updated this revision to Diff 308454.Nov 30 2020, 12:38 PM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)

Fix a comment

This revision was landed with ongoing or failed builds.Nov 30 2020, 12:44 PM
This revision was automatically updated to reflect the committed changes.