[llvm] 5698537 - Update basic deref API to account for possiblity of free [NFC]
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 19 16:37:12 PDT 2021
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 <mailto: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
> <https://github.com/llvm/llvm-project/commit/5698537f81a2ecf1166f41cab264b92af670aaa1>
> DIFF:
> 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
> <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.
>
> 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 <mailto:llvm-commits at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> <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/c562a146/attachment.html>
More information about the llvm-commits
mailing list