[llvm] 5698537 - Update basic deref API to account for possiblity of free [NFC]
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 22 12:15:36 PDT 2021
On Fri, Mar 19, 2021 at 7:37 PM Philip Reames <listmail at philipreames.com>
wrote:
>
> On 3/19/21 1:54 PM, Eric Christopher wrote:
>
>
>
> On Fri, Mar 19, 2021 at 2:17 PM Philip Reames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> Author: Philip Reames
>> Date: 2021-03-19T11:17:19-07:00
>> New Revision: 5698537f81a2ecf1166f41cab264b92af670aaa1
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/5698537f81a2ecf1166f41cab264b92af670aaa1
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/5698537f81a2ecf1166f41cab264b92af670aaa1.diff
>>
>> LOG: Update basic deref API to account for possiblity of free [NFC]
>>
>> This patch is plumbing to support work towards the goal outlined in the
>> recent llvm-dev post "[llvm-dev] RFC: Decomposing deref(N) into deref(N) +
>> nofree".
>>
>> The point of this change is purely to simplify iteration on other pieces
>> on way to making the switch. Rebuilding with a change to Value.h is slow
>> and painful, so I want to get the API change landed. Once that's done, I
>> plan to more closely audit each caller, add the inference rules in their
>> own patch, then post a patch with the langref changes and test diffs. The
>> value of the command line flag is that we can exercise the inference logic
>> in standalone patches without needing the whole switch ready to go just yet.
>>
>> Differential Revision: https://reviews.llvm.org/D98908
>>
>> Added:
>>
>>
>> Modified:
>> llvm/include/llvm/Analysis/MemoryBuiltins.h
>> llvm/include/llvm/IR/Value.h
>> llvm/lib/Analysis/BasicAliasAnalysis.cpp
>> llvm/lib/Analysis/CaptureTracking.cpp
>> llvm/lib/Analysis/Loads.cpp
>> llvm/lib/IR/Value.cpp
>> llvm/lib/Transforms/IPO/AttributorAttributes.cpp
>> llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>>
>> Removed:
>>
>>
>>
>>
>> ################################################################################
>> diff --git a/llvm/include/llvm/Analysis/MemoryBuiltins.h
>> b/llvm/include/llvm/Analysis/MemoryBuiltins.h
>> index c5428726995e..39ade20df53f 100644
>> --- a/llvm/include/llvm/Analysis/MemoryBuiltins.h
>> +++ b/llvm/include/llvm/Analysis/MemoryBuiltins.h
>> @@ -212,6 +212,10 @@ struct ObjectSizeOpts {
>> /// object size in Size if successful, and false otherwise. In this
>> context, by
>> /// object we mean the region of memory starting at Ptr to the end of the
>> /// underlying object pointed to by Ptr.
>> +///
>> +/// WARNING: The object size returned is the allocation size. This does
>> not
>> +/// imply dereferenceability at site of use since the object may be
>> freeed in
>> +/// between.
>> bool getObjectSize(const Value *Ptr, uint64_t &Size, const DataLayout
>> &DL,
>> const TargetLibraryInfo *TLI, ObjectSizeOpts Opts =
>> {});
>>
>>
>> diff --git a/llvm/include/llvm/IR/Value.h b/llvm/include/llvm/IR/Value.h
>> index 5a7e90aeb0f6..e9a9acfd69ba 100644
>> --- a/llvm/include/llvm/IR/Value.h
>> +++ b/llvm/include/llvm/IR/Value.h
>> @@ -743,8 +743,13 @@ class Value {
>> ///
>> /// If CanBeNull is set by this function the pointer can either be
>> null or be
>> /// dereferenceable up to the returned number of bytes.
>> + ///
>> + /// IF CanBeFreed is true, the pointer is known to be dereferenceable
>> at
>> + /// point of definition only. Caller must prove that allocation is not
>> + /// deallocated between point of definition and use.
>> uint64_t getPointerDereferenceableBytes(const DataLayout &DL,
>> - bool &CanBeNull) const;
>> + bool &CanBeNull,
>> + bool &CanBeFreed) const;
>>
>
> FWIW I'm not a fan of this particular pattern - I find things like:
>
> foo(dl, false, true)
>
> to be confusing and a lot less than readable as a pattern. Can we instead
> either use multiple routines, different interfaces, enums, etc?
>
> I think you may have misread the code. You seem to be thinking these are
> configuration params, but the actual code is using them for out params.
>
>
> Fair statement, it was late and I was extremely tired. Relatedly I'm not a
fan of multiple out params passed by reference. Perhaps this should return
a named struct instead that gives documented return values rather than
bools passed by reference?
:)
-eric
> Thanks!
>
> -eric
>
>
>>
>> /// Returns an alignment of the pointer value.
>> ///
>>
>> diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>> b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>> index 11fa4d2893e6..a8c5b9ca80e4 100644
>> --- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>> +++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
>> @@ -199,9 +199,11 @@ static uint64_t getMinimalExtentFrom(const Value &V,
>> // If we have dereferenceability information we know a lower bound for
>> the
>> // extent as accesses for a lower offset would be valid. We need to
>> exclude
>> // the "or null" part if null is a valid pointer.
>> - bool CanBeNull;
>> - uint64_t DerefBytes = V.getPointerDereferenceableBytes(DL, CanBeNull);
>> + bool CanBeNull, CanBeFreed;
>> + uint64_t DerefBytes =
>> + V.getPointerDereferenceableBytes(DL, CanBeNull, CanBeFreed);
>> DerefBytes = (CanBeNull && NullIsValidLoc) ? 0 : DerefBytes;
>> + DerefBytes = CanBeFreed ? 0 : DerefBytes;
>> // If queried with a precise location size, we assume that location
>> size to be
>> // accessed, thus valid.
>> if (LocSize.isPrecise())
>>
>> diff --git a/llvm/lib/Analysis/CaptureTracking.cpp
>> b/llvm/lib/Analysis/CaptureTracking.cpp
>> index b2fc6e603f9e..cf5e53b26b5d 100644
>> --- a/llvm/lib/Analysis/CaptureTracking.cpp
>> +++ b/llvm/lib/Analysis/CaptureTracking.cpp
>> @@ -68,8 +68,8 @@ bool CaptureTracker::isDereferenceableOrNull(Value *O,
>> const DataLayout &DL) {
>> if (auto *GEP = dyn_cast<GetElementPtrInst>(O))
>> if (GEP->isInBounds())
>> return true;
>> - bool CanBeNull;
>> - return O->getPointerDereferenceableBytes(DL, CanBeNull);
>> + bool CanBeNull, CanBeFreed;
>> + return O->getPointerDereferenceableBytes(DL, CanBeNull, CanBeFreed);
>> }
>>
>> namespace {
>>
>> diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
>> index 88e4c723331a..7279ed59c440 100644
>> --- a/llvm/lib/Analysis/Loads.cpp
>> +++ b/llvm/lib/Analysis/Loads.cpp
>> @@ -67,10 +67,12 @@ static bool isDereferenceableAndAlignedPointer(
>> Visited, MaxDepth);
>> }
>>
>> - bool CheckForNonNull = false;
>> + bool CheckForNonNull, CheckForFreed;
>> APInt KnownDerefBytes(Size.getBitWidth(),
>> - V->getPointerDereferenceableBytes(DL,
>> CheckForNonNull));
>> - if (KnownDerefBytes.getBoolValue() && KnownDerefBytes.uge(Size))
>> + V->getPointerDereferenceableBytes(DL,
>> CheckForNonNull,
>> +
>> CheckForFreed));
>> + if (KnownDerefBytes.getBoolValue() && KnownDerefBytes.uge(Size) &&
>> + !CheckForFreed)
>> if (!CheckForNonNull || isKnownNonZero(V, DL, 0, nullptr, CtxI, DT))
>> {
>> // As we recursed through GEPs to get here, we've incrementally
>> checked
>> // that each step advanced by a multiple of the alignment. If our
>> base is
>>
>> diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
>> index 92ffae18ae6f..cfb91b55f707 100644
>> --- a/llvm/lib/IR/Value.cpp
>> +++ b/llvm/lib/IR/Value.cpp
>> @@ -38,6 +38,11 @@
>>
>> using namespace llvm;
>>
>> +static cl::opt<unsigned> UseDerefAtPointSemantics(
>> + "use-dereferenceable-at-point-semantics", cl::Hidden,
>> cl::init(false),
>> + cl::desc("Deref attributes and metadata infer facts at definition
>> only"));
>> +
>> +
>> static cl::opt<unsigned> NonGlobalValueMaxNameSize(
>> "non-global-value-max-name-size", cl::Hidden, cl::init(1024),
>> cl::desc("Maximum size for the name of non-global values."));
>> @@ -724,11 +729,13 @@ Value::stripInBoundsOffsets(function_ref<void(const
>> Value *)> Func) const {
>> }
>>
>> uint64_t Value::getPointerDereferenceableBytes(const DataLayout &DL,
>> - bool &CanBeNull) const {
>> + bool &CanBeNull,
>> + bool &CanBeFreed) const {
>> assert(getType()->isPointerTy() && "must be pointer");
>>
>> uint64_t DerefBytes = 0;
>> CanBeNull = false;
>> + CanBeFreed = UseDerefAtPointSemantics;
>> if (const Argument *A = dyn_cast<Argument>(this)) {
>> DerefBytes = A->getDereferenceableBytes();
>> if (DerefBytes == 0) {
>> @@ -783,6 +790,7 @@ uint64_t Value::getPointerDereferenceableBytes(const
>> DataLayout &DL,
>> DerefBytes =
>> DL.getTypeStoreSize(AI->getAllocatedType()).getKnownMinSize();
>> CanBeNull = false;
>> + CanBeFreed = false;
>> }
>> } else if (auto *GV = dyn_cast<GlobalVariable>(this)) {
>> if (GV->getValueType()->isSized() && !GV->hasExternalWeakLinkage()) {
>> @@ -790,6 +798,7 @@ uint64_t Value::getPointerDereferenceableBytes(const
>> DataLayout &DL,
>> // CanBeNull flag.
>> DerefBytes =
>> DL.getTypeStoreSize(GV->getValueType()).getFixedSize();
>> CanBeNull = false;
>> + CanBeFreed = false;
>> }
>> }
>> return DerefBytes;
>>
>> diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
>> b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
>> index 21fa11aadea8..fa32a22059ac 100644
>> --- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
>> +++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
>> @@ -1750,8 +1750,9 @@ struct AANonNullImpl : AANonNull {
>>
>> AANonNull::initialize(A);
>>
>> - bool CanBeNull = true;
>> - if (V.getPointerDereferenceableBytes(A.getDataLayout(), CanBeNull)) {
>> + bool CanBeNull, CanBeFreed;
>> + if (V.getPointerDereferenceableBytes(A.getDataLayout(), CanBeNull,
>> + CanBeFreed)) {
>> if (!CanBeNull) {
>> indicateOptimisticFixpoint();
>> return;
>> @@ -3548,10 +3549,10 @@ struct AADereferenceableImpl : AADereferenceable {
>> const IRPosition &IRP = this->getIRPosition();
>> NonNullAA = &A.getAAFor<AANonNull>(*this, IRP, DepClassTy::NONE);
>>
>> - bool CanBeNull;
>> + bool CanBeNull, CanBeFreed;
>> takeKnownDerefBytesMaximum(
>> IRP.getAssociatedValue().getPointerDereferenceableBytes(
>> - A.getDataLayout(), CanBeNull));
>> + A.getDataLayout(), CanBeNull, CanBeFreed));
>>
>> bool IsFnInterface = IRP.isFnInterfaceKind();
>> Function *FnScope = IRP.getAnchorScope();
>> @@ -3661,8 +3662,9 @@ struct AADereferenceableFloating :
>> AADereferenceableImpl {
>> if (!Stripped && this == &AA) {
>> // Use IR information if we did not strip anything.
>> // TODO: track globally.
>> - bool CanBeNull;
>> - DerefBytes = Base->getPointerDereferenceableBytes(DL, CanBeNull);
>> + bool CanBeNull, CanBeFreed;
>> + DerefBytes =
>> + Base->getPointerDereferenceableBytes(DL, CanBeNull,
>> CanBeFreed);
>> T.GlobalState.indicatePessimisticFixpoint();
>> } else {
>> const DerefState &DS = AA.getState();
>>
>> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> index e6e90b915bb8..ae72123e3f00 100644
>> --- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> +++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
>> @@ -2593,8 +2593,8 @@ Instruction
>> *InstCombinerImpl::visitBitCast(BitCastInst &CI) {
>>
>> // If the source pointer is dereferenceable, then assume it points
>> to an
>> // allocated object and apply "inbounds" to the GEP.
>> - bool CanBeNull;
>> - if (Src->getPointerDereferenceableBytes(DL, CanBeNull)) {
>> + bool CanBeNull, CanBeFreed;
>> + if (Src->getPointerDereferenceableBytes(DL, CanBeNull,
>> CanBeFreed)) {
>> // In a non-default address space (not 0), a null pointer can
>> not be
>> // assumed inbounds, so ignore that case
>> (dereferenceable_or_null).
>> // The reason is that 'null' is not treated
>> diff erently in these address
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210322/6bcad7a9/attachment.html>
More information about the llvm-commits
mailing list