From 106a5eaff95125add4b4b4bc66eebd03439722c4 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Wed, 30 Oct 2019 17:21:53 -0500 Subject: [PATCH] [Attributor] Really use the executed-context Before we did not follow casts and geps when we looked at the users of a pointer in the pointers must-be-executed-context. This caused us to fail to determine if it was accessed for sure. With this change we follow such users now. The above extension exposed problems in getKnownNonNullAndDerefBytesForUse which did not always check what the base pointer was. We also did not handle negative offsets as conservative as we have to without explicit loop handling. Finally, we should not derive a huge number if we access a pointer that was traversed backwards first. The problems exposed by this functional change are already tested in the existing test cases as is the functional change. Differential Revision: https://reviews.llvm.org/D69647 --- lib/Transforms/IPO/Attributor.cpp | 43 +++++++++++++------ .../Transforms/FunctionAttrs/arg_nocapture.ll | 6 +-- .../FunctionAttrs/dereferenceable.ll | 8 ++-- test/Transforms/FunctionAttrs/nocapture.ll | 6 ++- test/Transforms/FunctionAttrs/nosync.ll | 10 ++--- .../InferFunctionAttrs/dereferenceable.ll | 8 +--- 6 files changed, 47 insertions(+), 34 deletions(-) diff --git a/lib/Transforms/IPO/Attributor.cpp b/lib/Transforms/IPO/Attributor.cpp index ca0ff3ab03e..6589b16348f 100644 --- a/lib/Transforms/IPO/Attributor.cpp +++ b/lib/Transforms/IPO/Attributor.cpp @@ -726,19 +726,16 @@ struct AAFromMustBeExecutedContext : public Base { MustBeExecutedContextExplorer &Explorer = A.getInfoCache().getMustBeExecutedContextExplorer(); - SetVector NextUses; - auto EIt = Explorer.begin(CtxI), EEnd = Explorer.end(CtxI); - for (const Use *U : Uses) { + for (unsigned u = 0 ; u < Uses.size(); ++u) { + const Use *U = Uses[u]; if (const Instruction *UserI = dyn_cast(U->getUser())) { bool Found = Explorer.findInContextOf(UserI, EIt, EEnd); if (Found && Base::followUse(A, U, UserI)) for (const Use &Us : UserI->uses()) - NextUses.insert(&Us); + Uses.insert(&Us); } } - for (const Use *U : NextUses) - Uses.insert(U); return BeforeState == S ? ChangeStatus::UNCHANGED : ChangeStatus::CHANGED; } @@ -1550,25 +1547,41 @@ static int64_t getKnownNonNullAndDerefBytesForUse( return DerefAA.getKnownDereferenceableBytes(); } + // We need to follow common pointer manipulation uses to the accesses they + // feed into. We can try to be smart to avoid looking through things we do not + // like for now, e.g., non-inbounds GEPs. + if (isa(I)) { + TrackUse = true; + return 0; + } + if (auto *GEP = dyn_cast(I)) + if (GEP->hasAllZeroIndices() || + (GEP->isInBounds() && GEP->hasAllConstantIndices())) { + TrackUse = true; + return 0; + } + int64_t Offset; if (const Value *Base = getBasePointerOfAccessPointerOperand(I, Offset, DL)) { if (Base == &AssociatedValue && getPointerOperand(I) == UseV) { int64_t DerefBytes = - Offset + (int64_t)DL.getTypeStoreSize(PtrTy->getPointerElementType()); + (int64_t)DL.getTypeStoreSize(PtrTy->getPointerElementType()) + Offset; IsNonNull |= !NullPointerIsDefined; - return DerefBytes; + return std::max(int64_t(0), DerefBytes); } } if (const Value *Base = GetPointerBaseWithConstantOffset(UseV, Offset, DL, /*AllowNonInbounds*/ false)) { - auto &DerefAA = - A.getAAFor(QueryingAA, IRPosition::value(*Base)); - IsNonNull |= (!NullPointerIsDefined && DerefAA.isKnownNonNull()); - IsNonNull |= (!NullPointerIsDefined && (Offset != 0)); - int64_t DerefBytes = DerefAA.getKnownDereferenceableBytes(); - return std::max(int64_t(0), DerefBytes - Offset); + if (Base == &AssociatedValue) { + auto &DerefAA = + A.getAAFor(QueryingAA, IRPosition::value(*Base)); + IsNonNull |= (!NullPointerIsDefined && DerefAA.isKnownNonNull()); + IsNonNull |= (!NullPointerIsDefined && (Offset != 0)); + int64_t DerefBytes = DerefAA.getKnownDereferenceableBytes(); + return std::max(int64_t(0), DerefBytes - std::max(int64_t(0), Offset)); + } } return 0; @@ -1586,6 +1599,8 @@ struct AANonNullImpl : AANonNull { if (!NullIsDefined && hasAttr({Attribute::NonNull, Attribute::Dereferenceable})) indicateOptimisticFixpoint(); + else if (isa(getAssociatedValue())) + indicatePessimisticFixpoint(); else AANonNull::initialize(A); } diff --git a/test/Transforms/FunctionAttrs/arg_nocapture.ll b/test/Transforms/FunctionAttrs/arg_nocapture.ll index 021c4df533e..18a4f767147 100644 --- a/test/Transforms/FunctionAttrs/arg_nocapture.ll +++ b/test/Transforms/FunctionAttrs/arg_nocapture.ll @@ -116,7 +116,7 @@ entry: ; ; CHECK: define dereferenceable_or_null(8) i64* @scc_B(double* readnone returned dereferenceable_or_null(8) "no-capture-maybe-returned" %a) ; -; CHECK: define dereferenceable_or_null(2) i8* @scc_C(i16* readnone returned dereferenceable_or_null(2) "no-capture-maybe-returned" %a) +; CHECK: define dereferenceable_or_null(4) i8* @scc_C(i16* readnone returned dereferenceable_or_null(4) "no-capture-maybe-returned" %a) ; ; float *scc_A(int *a) { ; return (float*)(a ? (int*)scc_A((int*)scc_B((double*)scc_C((short*)a))) : a); @@ -260,7 +260,7 @@ entry: ; } ; ; There should *not* be a no-capture attribute on %a -; CHECK: define nonnull i64* @not_captured_but_returned_1(i64* writeonly "no-capture-maybe-returned" %a) +; CHECK: define nonnull dereferenceable(8) i64* @not_captured_but_returned_1(i64* nonnull writeonly dereferenceable(16) "no-capture-maybe-returned" %a) define i64* @not_captured_but_returned_1(i64* %a) #0 { entry: %add.ptr = getelementptr inbounds i64, i64* %a, i64 1 @@ -320,7 +320,7 @@ entry: ; } ; ; There should *not* be a no-capture attribute on %a -; CHECK: define nonnull i64* @negative_test_not_captured_but_returned_call_1a(i64* writeonly "no-capture-maybe-returned" %a) +; CHECK: define nonnull dereferenceable(8) i64* @negative_test_not_captured_but_returned_call_1a(i64* writeonly "no-capture-maybe-returned" %a) define i64* @negative_test_not_captured_but_returned_call_1a(i64* %a) #0 { entry: %call = call i64* @not_captured_but_returned_1(i64* %a) diff --git a/test/Transforms/FunctionAttrs/dereferenceable.ll b/test/Transforms/FunctionAttrs/dereferenceable.ll index a8608478430..973f7f517e6 100644 --- a/test/Transforms/FunctionAttrs/dereferenceable.ll +++ b/test/Transforms/FunctionAttrs/dereferenceable.ll @@ -30,8 +30,8 @@ define i32* @test3_1(i32* dereferenceable(8) %0) local_unnamed_addr { } define i32* @test3_2(i32* dereferenceable_or_null(32) %0) local_unnamed_addr { -; FIXME: Argument should be mark dereferenceable because of GEP `inbounds`. -; ATTRIBUTOR: define nonnull dereferenceable(16) i32* @test3_2(i32* readnone dereferenceable_or_null(32) "no-capture-maybe-returned" %0) +; FIXME: We should not have both deref(x) and deref_or_null(y) with x >= y. +; ATTRIBUTOR: define nonnull dereferenceable(16) i32* @test3_2(i32* nonnull readnone dereferenceable(32) dereferenceable_or_null(32) "no-capture-maybe-returned" %0) %ret = getelementptr inbounds i32, i32* %0, i64 4 ret i32* %ret } @@ -196,8 +196,8 @@ define i32* @f7_3() { } define i32* @test_for_minus_index(i32* %p) { -; FIXME: This should be define nonnull dereferenceable(8) i32* @test_for_minus_index(i32* nonnull %p) -; ATTRIBUTOR: define nonnull dereferenceable(8) i32* @test_for_minus_index(i32* writeonly "no-capture-maybe-returned" %p) +; FIXME: This should have a return dereferenceable(8) but we need to make sure it will work in loops as well. +; ATTRIBUTOR: define nonnull i32* @test_for_minus_index(i32* nonnull writeonly "no-capture-maybe-returned" %p) %q = getelementptr inbounds i32, i32* %p, i32 -2 store i32 1, i32* %q ret i32* %q diff --git a/test/Transforms/FunctionAttrs/nocapture.ll b/test/Transforms/FunctionAttrs/nocapture.ll index f8927a60cb1..4fb45589648 100644 --- a/test/Transforms/FunctionAttrs/nocapture.ll +++ b/test/Transforms/FunctionAttrs/nocapture.ll @@ -306,7 +306,8 @@ define i1 @captureICmpRev(i32* %x) { ret i1 %1 } -; EITHER: define i1 @nocaptureInboundsGEPICmp(i32* nocapture readnone %x) +; FNATTR: define i1 @nocaptureInboundsGEPICmp(i32* nocapture readnone %x) +; ATTRIBUTOR: define i1 @nocaptureInboundsGEPICmp(i32* nocapture nonnull readnone %x) define i1 @nocaptureInboundsGEPICmp(i32* %x) { %1 = getelementptr inbounds i32, i32* %x, i32 5 %2 = bitcast i32* %1 to i8* @@ -314,7 +315,8 @@ define i1 @nocaptureInboundsGEPICmp(i32* %x) { ret i1 %3 } -; EITHER: define i1 @nocaptureInboundsGEPICmpRev(i32* nocapture readnone %x) +; FNATTR: define i1 @nocaptureInboundsGEPICmpRev(i32* nocapture readnone %x) +; ATTRIBUTOR: define i1 @nocaptureInboundsGEPICmpRev(i32* nocapture nonnull readnone %x) define i1 @nocaptureInboundsGEPICmpRev(i32* %x) { %1 = getelementptr inbounds i32, i32* %x, i32 5 %2 = bitcast i32* %1 to i8* diff --git a/test/Transforms/FunctionAttrs/nosync.ll b/test/Transforms/FunctionAttrs/nosync.ll index 381c5abb261..253b34d8308 100644 --- a/test/Transforms/FunctionAttrs/nosync.ll +++ b/test/Transforms/FunctionAttrs/nosync.ll @@ -28,7 +28,7 @@ target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" ; FNATTR: Function Attrs: norecurse nounwind optsize readnone ssp uwtable ; FNATTR-NEXT: define nonnull i32* @foo(%struct.ST* readnone %s) ; ATTRIBUTOR: Function Attrs: nofree nosync nounwind optsize readnone ssp uwtable -; ATTRIBUTOR-NEXT: define nonnull i32* @foo(%struct.ST* readnone "no-capture-maybe-returned" %s) +; ATTRIBUTOR-NEXT: define nonnull i32* @foo(%struct.ST* nonnull readnone "no-capture-maybe-returned" %s) define i32* @foo(%struct.ST* %s) nounwind uwtable readnone optsize ssp { entry: %arrayidx = getelementptr inbounds %struct.ST, %struct.ST* %s, i64 1, i32 2, i32 1, i64 5, i64 13 @@ -224,7 +224,7 @@ define void @scc2(i32* %0) noinline nounwind uwtable { ; FNATTR: Function Attrs: nofree norecurse nounwind ; FNATTR-NEXT: define void @foo1(i32* nocapture %0, %"struct.std::atomic"* nocapture %1) ; ATTRIBUTOR-NOT: nosync -; ATTRIBUTOR: define void @foo1(i32* nocapture nonnull writeonly dereferenceable(4) %0, %"struct.std::atomic"* nocapture writeonly %1) +; ATTRIBUTOR: define void @foo1(i32* nocapture nonnull writeonly dereferenceable(4) %0, %"struct.std::atomic"* nocapture nonnull writeonly dereferenceable(1) %1) define void @foo1(i32* %0, %"struct.std::atomic"* %1) { store i32 100, i32* %0, align 4 @@ -237,7 +237,7 @@ define void @foo1(i32* %0, %"struct.std::atomic"* %1) { ; FNATTR: Function Attrs: nofree norecurse nounwind ; FNATTR-NEXT: define void @bar(i32* nocapture readnone %0, %"struct.std::atomic"* nocapture readonly %1) ; ATTRIBUTOR-NOT: nosync -; ATTRIBUTOR: define void @bar(i32* nocapture readnone %0, %"struct.std::atomic"* nocapture readonly %1) +; ATTRIBUTOR: define void @bar(i32* nocapture readnone %0, %"struct.std::atomic"* nocapture nonnull readonly dereferenceable(1) %1) define void @bar(i32* %0, %"struct.std::atomic"* %1) { %3 = getelementptr inbounds %"struct.std::atomic", %"struct.std::atomic"* %1, i64 0, i32 0, i32 0 br label %4 @@ -257,7 +257,7 @@ define void @bar(i32* %0, %"struct.std::atomic"* %1) { ; FNATTR: Function Attrs: nofree norecurse nounwind ; FNATTR-NEXT: define void @foo1_singlethread(i32* nocapture %0, %"struct.std::atomic"* nocapture %1) ; ATTRIBUTOR: Function Attrs: nofree nosync nounwind willreturn -; ATTRIBUTOR: define void @foo1_singlethread(i32* nocapture nonnull writeonly dereferenceable(4) %0, %"struct.std::atomic"* nocapture writeonly %1) +; ATTRIBUTOR: define void @foo1_singlethread(i32* nocapture nonnull writeonly dereferenceable(4) %0, %"struct.std::atomic"* nocapture nonnull writeonly dereferenceable(1) %1) define void @foo1_singlethread(i32* %0, %"struct.std::atomic"* %1) { store i32 100, i32* %0, align 4 @@ -270,7 +270,7 @@ define void @foo1_singlethread(i32* %0, %"struct.std::atomic"* %1) { ; FNATTR: Function Attrs: nofree norecurse nounwind ; FNATTR-NEXT: define void @bar_singlethread(i32* nocapture readnone %0, %"struct.std::atomic"* nocapture readonly %1) ; ATTRIBUTOR: Function Attrs: nofree nosync nounwind -; ATTRIBUTOR: define void @bar_singlethread(i32* nocapture readnone %0, %"struct.std::atomic"* nocapture readonly %1) +; ATTRIBUTOR: define void @bar_singlethread(i32* nocapture readnone %0, %"struct.std::atomic"* nocapture nonnull readonly dereferenceable(1) %1) define void @bar_singlethread(i32* %0, %"struct.std::atomic"* %1) { %3 = getelementptr inbounds %"struct.std::atomic", %"struct.std::atomic"* %1, i64 0, i32 0, i32 0 br label %4 diff --git a/test/Transforms/InferFunctionAttrs/dereferenceable.ll b/test/Transforms/InferFunctionAttrs/dereferenceable.ll index 17c76f11d6c..25412051a7f 100644 --- a/test/Transforms/InferFunctionAttrs/dereferenceable.ll +++ b/test/Transforms/InferFunctionAttrs/dereferenceable.ll @@ -8,9 +8,7 @@ define <4 x double> @PR21780(double* %ptr) { ; CHECK-LABEL: @PR21780(double* %ptr) -; FIXME: this should be @PR21780(double* nonnull dereferenceable(32) %ptr) -; trakcing use of GEP in Attributor would fix this problem. -; ATTRIBUTOR-LABEL: @PR21780(double* nocapture nonnull readonly dereferenceable(8) %ptr) +; ATTRIBUTOR-LABEL: @PR21780(double* nocapture nonnull readonly dereferenceable(32) %ptr) ; GEP of index 0 is simplified away. %arrayidx1 = getelementptr inbounds double, double* %ptr, i64 1 @@ -33,9 +31,7 @@ define <4 x double> @PR21780(double* %ptr) { define double @PR21780_only_access3_with_inbounds(double* %ptr) { ; CHECK-LABEL: @PR21780_only_access3_with_inbounds(double* %ptr) -; FIXME: this should be @PR21780_only_access3_with_inbounds(double* nonnull dereferenceable(32) %ptr) -; trakcing use of GEP in Attributor would fix this problem. -; ATTRIBUTOR-LABEL: @PR21780_only_access3_with_inbounds(double* nocapture readonly %ptr) +; ATTRIBUTOR-LABEL: @PR21780_only_access3_with_inbounds(double* nocapture nonnull readonly dereferenceable(32) %ptr) %arrayidx3 = getelementptr inbounds double, double* %ptr, i64 3 %t3 = load double, double* %arrayidx3, align 8