[llvm] 5698537 - Update basic deref API to account for possiblity of free [NFC]
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 19 13:54:57 PDT 2021
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?
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/20210319/d833782c/attachment.html>
More information about the llvm-commits
mailing list