NoFree argument attribute.

[sstefan1]

Summary

Deducing nofree atrribute for function arguments.

[sstefan1]

There should be other test changes, will update tomorrow.

[jdoerfert]

I'm glad you work on this, this will be helpful for h2s and also deref analysis. I added some comments.

llvm/lib/IR/Verifier.cpp

1505

remove it completely please

llvm/lib/Transforms/IPO/Attributor.cpp

1419

Look at the function attribute first, as long as it is set we do not have to prove anything but can just keep the optimistic state.

1459

This above is not necessary if you use the function of the argument first to justify nofree.

1476

unknown users need to be treated as potential frees.

1477

I doubt calling indicateOptimisticFixpoint is correct.

1513

I would put the logic that follows uses into the floating version and make everything that is not function scope (function & call site) derive from floating. (We need to have a generic use visitor but that is a different story)

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

270

We need tests where one argument is freed and one is not. Also where either one of two is freed but we don't know which (e.g., due to a phi). And we need uncertainty tests, e.g. the argument escapes and the function (wrt. to the argument) is not nofree.

[sstefan1]

llvm/lib/Transforms/IPO/Attributor.cpp

1419

Ok, I agree. This is how I should have done it.

1513

I can do that. Just want to make sure I understand what you are saying. I should keep the current function & call site implementations, right?

I also thought about generic use visitor, I'll think about it and maybe write a patch this week.

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

270

I will add more tests soon. Actually a lot of tests will be affected, so maybe we are already covering some of the things you are mentioning.

[jdoerfert]

llvm/lib/Transforms/IPO/Attributor.cpp

1513

Keep function & call site as is, yes.

If you want to work on a use visitor that is fine with me. Having edge-wise liveness information is also up there on the list of things I'd like to have.

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

270

OK, we could add them either way to make it explicit ;)

[sstefan1]

• update tests

[jdoerfert]

I think the logic is fine, some minor comments though. Also, could you update the use in AAHeapToStackImpl::updateImpl to ask for the call site argument instead (with test for the improvement if we don't have one yet)?

llvm/lib/IR/Verifier.cpp

1555

LangRef also needs to be updated I guess.

llvm/lib/Transforms/IPO/Attributor.cpp

1419

F = getIRPosition().getAnchorScope()

1422

Why do we need an argument in the first place here (and why do you dereference it in 1419 before you check it for null in 1421, it can be null btw.).

1430

(Arg->getParent()) -> F

1450

use cast here. Users *have* to be instructions or we have way more problems (and we want it to explode if a user is not an instruction).

1456

you need to treat uses in the operand bundle as "freeing", so, isCalleeOperand -> ok, isArgOperand -> test further, if neither -> not nofree.

1458

no need for this check

1546

overwrite manifest here to ensure we do not add "no-free" to the return value even if we derive it. We can actually derive it as we do not need to restrict it to arguments in the AANoFreeFloating::update.

4427

leftover

[sstefan1]

• addressing comments

[jdoerfert]

quick feedback

llvm/lib/Transforms/IPO/Attributor.cpp

1433

Why do we need an argument and not just the associated value?

1478

Allow PHI nodes and selects as well. Same as bitcast above.

1546

This is still open, see my "why arguments" comment above.

Btw. later we can actually add "nofree" to return values (and call site returns) as it can help to keep dereferenceable. But that is for a follow up patch after we get dereferenceable_globally

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

267

ATTRIBUTOR: check line missing

Also replace the CHECK with ATTRIBUTOR

[sstefan1]

• minor updates.

[jdoerfert]

Small nits. Otherwise LGTM.
Also, make sure we have a test with 2 pointer arguments, one freed, one no-free annotated.

llvm/lib/Transforms/IPO/Attributor.cpp

1421

Replace IRPosition::function(*F)) with IRPosition::function_scope(IRP).
IRP is getIRPosition(). That should remove the need to look at the call site explicitly below.

1442

Hoist this out of the loop

1457

I don't think this is necessary but fine with me. (should be implied and checked by the call site argument attr).

[sstefan1]

• update tests

[thakis]

This breaks tests on all bots. Please fix quickly or revert, and please run tests locally before committing.

[jdoerfert]

In D67886#1731223, @thakis wrote:

This breaks tests on all bots. Please fix quickly or revert, and please run tests locally before committing.

You missed to update InferFunctionAttrs/dereferenceable.ll

/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm-project/llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:11:21: error: ATTRIBUTOR-LABEL: expected string not found in input
; ATTRIBUTOR-LABEL: @PR21780(double* nocapture nonnull readonly dereferenceable(32) %ptr)
                    ^
<stdin>:1:1: note: scanning from here
; ModuleID = '<stdin>'
^
<stdin>:5:21: note: possible intended match here
define <4 x double> @PR21780(double* nocapture nofree nonnull readonly dereferenceable(32) %ptr) #0 {

[sstefan1]

Yes, I missed that one. Sorry for the mess.

llvm/lib/IR/Verifier.cpp

 static bool isFuncOnlyAttr(Attribute::AttrKind Kind) {
   switch (Kind) {
   case Attribute::NoReturn:
   case Attribute::NoSync:
   case Attribute::WillReturn:
   case Attribute::NoCfCheck:
   case Attribute::NoUnwind:
   case Attribute::NoInline:
-  case Attribute::NoFree:
+  //case Attribute::NoFree:
   case Attribute::AlwaysInline:
   case Attribute::OptimizeForSize:
   case Attribute::StackProtect:
   case Attribute::StackProtectReq:
   case Attribute::StackProtectStrong:
   case Attribute::SafeStack:
   case Attribute::ShadowCallStack:
   case Attribute::NoRedZone:
static bool isFuncOnlyAttr(Attribute::AttrKind Kind) {
   }
   return false;
 }
 
 /// Return true if this is a function attribute that can also appear on
 /// arguments.
 static bool isFuncOrArgAttr(Attribute::AttrKind Kind) {
   return Kind == Attribute::ReadOnly || Kind == Attribute::WriteOnly ||
-         Kind == Attribute::ReadNone;
+         Kind == Attribute::ReadNone || Kind == Attribute::NoFree;
 }
 
 void Verifier::verifyAttributeTypes(AttributeSet Attrs, bool IsFunction,
                                     const Value *V) {
   for (Attribute A : Attrs) {
     if (A.isStringAttribute())
       continue;
 
     if (isFuncOnlyAttr(A.getKindAsEnum())) {

llvm/lib/Transforms/IPO/Attributor.cpp

ChangeStatus updateImpl(Attributor &A) override {
     return clampStateAndIndicateChange(
         getState(), static_cast<const AANoFree::StateType &>(FnAA.getState()));
   }
 
   /// See AbstractAttribute::trackStatistics()
   void trackStatistics() const override { STATS_DECLTRACK_CS_ATTR(nofree); }
 };
 
+/// NoFree attribute for a call site argument.
+struct AANoFreeArgument final : AANoFreeImpl {
+  AANoFreeArgument(const IRPosition &IRP) : AANoFreeImpl(IRP) {}
+
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override { STATS_DECLTRACK_ARG_ATTR(nofree) }
+
+  /// See Abstract Attribute::updateImpl(...).
+  ChangeStatus updateImpl(Attributor &A) override {
+    Argument *Arg = getAssociatedArgument();
+    Function *F = Arg->getParent();
+
+    if (!Arg)
+      return indicatePessimisticFixpoint();
+
+    const auto *TLI =
+        A.getInfoCache().getTargetLibraryInfoForFunction(*(Arg->getParent()));
+
+    SmallPtrSet<const Use *, 8> Visited;
+    SmallVector<const Use *, 8> Worklist;
+
+    for (Use &U : Arg->uses())
+      Worklist.push_back(&U);
+
+    while (!Worklist.empty()) {
+      const Use *U = Worklist.pop_back_val();
+      if (!Visited.insert(U).second)
+        continue;
+
+      auto *UserI = U->getUser();
+
+      const AAIsDead &LivenessAA =
+          A.getAAFor<AAIsDead>(*this, IRPosition::function(*F));
+
+      if(LivenessAA.isAssumedDead(dyn_cast<Instruction>(UserI)))
+        continue;
+
+      // Use is freed, we are done.
+      if (auto *CB = dyn_cast<CallBase>(UserI)) {
+        if (!CB->isArgOperand(U))
+          continue;
+        if (isFreeCall(UserI, TLI))
+          return indicatePessimisticFixpoint();
+
+        Function *CalledF = CB->getCalledFunction();
+
+        const auto &NoFreeFunc = A.getAAFor<AANoFree>(
+            *this, IRPosition::function(*CalledF));
+
+        if (NoFreeFunc.isAssumedNoFree())
+          continue;
+
+        unsigned ArgNo = U - CB->arg_begin();
+        Argument *UseArg = CalledF->getArg(ArgNo);
+
+        const auto &NoFreeArg =
+            A.getAAFor<AANoFree>(*this, IRPosition::argument(*UseArg));
+
+        if (NoFreeArg.isAssumedNoFree())
+          continue;
+
+        return indicatePessimisticFixpoint();
+      }
+
+      if (isa<GetElementPtrInst>(UserI) || isa<BitCastInst>(UserI))
+        for (Use &U : UserI->uses())
+          Worklist.push_back(&U);
+    }
+    return indicateOptimisticFixpoint();
+  }
+};
+
+/// NoFree attribute for call site arguments.
+struct AANoFreeCallSiteArgument final : AANoFreeImpl {
+  AANoFreeCallSiteArgument(const IRPosition &IRP) : AANoFreeImpl(IRP) {}
+  
+  /// See AbstractAttribute::updateImpl(...).
+  ChangeStatus updateImpl(Attributor &A) override {
+    // TODO: Once we have call site specific value information we can provide
+    //       call site specific liveness information and then it makes
+    //       sense to specialize attributes for call sites arguments instead of
+    //       redirecting requests to the callee argument.
+    Argument *Arg = getAssociatedArgument();
+    if (!Arg)
+      return indicatePessimisticFixpoint();
+    const IRPosition &ArgPos = IRPosition::argument(*Arg);
+    auto &ArgAA = A.getAAFor<AANoFree>(*this, ArgPos);
+    return clampStateAndIndicateChange(
+        getState(),
+        static_cast<const AANoFree::StateType &>(ArgAA.getState()));
+  }
+
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override{STATS_DECLTRACK_CSARG_ATTR(nofree)};
+};
+
+/// NoFree attribute for floating values.
+struct AANoFreeFloating final : AANoFreeImpl {
+  AANoFreeFloating(const IRPosition &IRP) : AANoFreeImpl(IRP) {}
+
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override {
+    STATS_DECLTRACK_FLOATING_ATTR(nofree)
+  }
+};
+
+/// NoCapture attribute for function return value.
+struct AANoFreeReturned final : AANoFreeImpl {
+  AANoFreeReturned(const IRPosition &IRP) : AANoFreeImpl(IRP) {
+    llvm_unreachable("NoFree is not applicable to function returns!");
+  }
+
+  /// See AbstractAttribute::initialize(...).
+  void initialize(Attributor &A) override {
+    llvm_unreachable("NoFree is not applicable to function returns!");
+  }
+
+  /// See AbstractAttribute::updateImpl(...).
+  ChangeStatus updateImpl(Attributor &A) override {
+    llvm_unreachable("NoFree is not applicable to function returns!");
+  }
+
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override {}
+};
+
+/// NoCapture attribute deduction for a call site return value.
+struct AANoFreeCallSiteReturned final : AANoFreeImpl {
+  AANoFreeCallSiteReturned(const IRPosition &IRP) : AANoFreeImpl(IRP) {}
+
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override {
+    STATS_DECLTRACK_CSRET_ATTR(nofree)
+  }
+};
 /// ------------------------ NonNull Argument Attribute ------------------------
 struct AANonNullImpl : AANonNull {
   AANonNullImpl(const IRPosition &IRP) : AANonNull(IRP) {}
 
   /// See AbstractAttribute::initialize(...).
   void initialize(Attributor &A) override {
     if (hasAttr({Attribute::NonNull, Attribute::Dereferenceable}))
       indicateOptimisticFixpoint();
     else
       AANonNull::initialize(A);
   }
if (Arg.getType()->isPointerTy()) {
       // Every argument with pointer type might be marked dereferenceable.
       getOrCreateAAFor<AADereferenceable>(ArgPos);
 
       // Every argument with pointer type might be marked align.
       getOrCreateAAFor<AAAlign>(ArgPos);
 
       // Every argument with pointer type might be marked nocapture.
       getOrCreateAAFor<AANoCapture>(ArgPos);
+
+      // Every argument with pointer type might be marked nofree.
+      getOrCreateAAFor<AANoFree>(ArgPos);
     }
   }
 
   auto CallSitePred = [&](Instruction &I) -> bool {
     CallSite CS(&I);
     if (CS.getCalledFunction()) {
       for (int i = 0, e = CS.getCalledFunction()->arg_size(); i < e; i++) {
 
if (CS.getCalledFunction()) {
         // Call site argument attribute "no-alias".
         getOrCreateAAFor<AANoAlias>(CSArgPos);
 
         // Call site argument attribute "dereferenceable".
         getOrCreateAAFor<AADereferenceable>(CSArgPos);
 
         // Call site argument attribute "align".
         getOrCreateAAFor<AAAlign>(CSArgPos);
+
+        // Call site  argument attribute  "nofree".
+        getOrCreateAAFor<AANoFree>(CSArgPos);
       }
     }
     return true;
   };
 
   auto &OpcodeInstMap = InfoCache.getOpcodeInstMapForFunction(F);
   bool Success, AnyDead = false;
   Success = checkForAllInstructionsImpl(
switch (IRP.getPositionKind()) {                                           \
       SWITCH_PK_CREATE(CLASS, IRP, IRP_FUNCTION, Function)                     \
     }                                                                          \
     AA->initialize(A);                                                         \
     return *AA;                                                                \
   }
 
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoUnwind)
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoSync)
-CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoFree)
+// CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoFree)
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoRecurse)
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAWillReturn)
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoReturn)
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAIsDead)
 CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAReturnedValues)
 
 CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANonNull)
 CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoAlias)
 CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AADereferenceable)
 CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAAlign)
 CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoCapture)
 
 CREATE_ALL_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAValueSimplify)
+CREATE_ALL_ABSTRACT_ATTRIBUTE_FOR_POSITION(AANoFree)
 
 CREATE_FUNCTION_ONLY_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAHeapToStack)
 
 #undef CREATE_FUNCTION_ABSTRACT_ATTRIBUTE_FOR_POSITION
 #undef CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION
 #undef CREATE_ALL_ABSTRACT_ATTRIBUTE_FOR_POSITION
 #undef SWITCH_PK_CREATE
 #undef SWITCH_PK_INV
 
 INITIALIZE_PASS_BEGIN(AttributorLegacyPass, "attributor",
                       "Deduce and propagate attributes", false, false)
 INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
 INITIALIZE_PASS_END(AttributorLegacyPass, "attributor",
                     "Deduce and propagate attributes", false, false)

llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll

 ; RUN: opt -functionattrs --disable-nofree-inference=false -S < %s | FileCheck %s --check-prefix=FNATTR
-; RUN: opt -attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=4 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
+; RUN: opt -passes=attributor --attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=4 -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 ; Test cases specifically designed for the "nofree" function attribute.
 ; We use FIXME's to indicate problems and missing attributes.
 
 ; Free functions
 declare void @free(i8* nocapture) local_unnamed_addr #1
 declare noalias i8* @realloc(i8* nocapture, i64) local_unnamed_addr #0
 declare void @_ZdaPv(i8*) local_unnamed_addr #2
 
+; declare void @helper(i8* nocapture nofree)
+
 
 ; TEST 1 (positive case)
 ; FNATTR: Function Attrs: noinline norecurse nounwind readnone uwtable
 ; FNATTR-NEXT: define void @only_return()
 ; ATTRIBUTOR: Function Attrs: nofree noinline nosync nounwind uwtable
 ; ATTRIBUTOR-NEXT: define void @only_return()
 define void @only_return() #0 {
     ret void

 ; <label>:4:                                      ; preds = %3, %1
   ret void
 }
 
 
 ; TEST 6 (negative case)
 ; Call realloc
 ; FNATTR: Function Attrs: noinline nounwind uwtable
-; FNATTR-NEXT: define noalias i8* @call_realloc(i8* nocapture %0, i64 %1) local_unnamed_addr
+; FNATTR-NEXT: define noalias i8* @call_realloc(i8* nocapture %0, i64 %1, i8* nocapture readnone %2) local_unnamed_addr
 ; ATTRIBUTOR: Function Attrs: noinline nounwind uwtable
 ; ATTRIBUTOR-NOT: nofree
-; ATTRIBUTOR-NEXT: define noalias i8* @call_realloc(i8* nocapture %0, i64 %1) local_unnamed_addr
-define noalias i8* @call_realloc(i8* nocapture %0, i64 %1) local_unnamed_addr #0 {
+; ATTRIBUTOR-NEXT: define noalias i8* @call_realloc(i8* nocapture %0, i64 %1, i8* nocapture nofree %2) local_unnamed_addr
+define noalias i8* @call_realloc(i8* nocapture %0, i64 %1, i8* nocapture %2) local_unnamed_addr #0 {
     %ret = tail call i8* @realloc(i8* %0, i64 %1) #2
     ret i8* %ret
 }
 
 
 ; TEST 7 (positive case)
 ; Call function declaration with "nofree"
 

 ; FNATTRS-NEXT: define void @f2()
 ; ATTRIBUTOR: Function Attrs: nofree noinline nosync nounwind uwtable
 ; ATTRIBUTOR-NEXT: define void @f2()
 define void @f2() #0 {
     tail call void @f1()
     ret void
 }
 
+; TEST 12 NoFree argument - positive.
+; CHECK: define double @test12(double* nocapture nofree readonly %a) 
+define double @test12(double* nocapture readonly %a) {
+entry:
+  %0 = load double, double* %a, align 8
+  %call = tail call double @cos(double %0) #2
+  ret double %call
+}
+
+declare double @cos(double) local_unnamed_addr #2
+
+; TEST 13 NoFree argument - positive.
+; CHECK: define noalias i32* @test13(i64* nocapture nofree readonly %a) {
+define noalias i32* @test13(i64* nocapture readonly %a) {
+entry:
+  %0 = load i64, i64* %a, align 8
+  %call = tail call noalias i8* @malloc(i64 %0) #2
+  %1 = bitcast i8* %call to i32*
+  ret i32* %1
+}
+
+declare noalias i8* @malloc(i64)
 
 attributes #0 = { nounwind uwtable noinline }
 attributes #1 = { nounwind }
 attributes #2 = { nobuiltin nounwind }