[PATCH] D67886: NoFree argument attribute.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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)?



================
Comment at: llvm/lib/IR/Verifier.cpp:1554
+         Kind == Attribute::ReadNone || Kind == Attribute::NoFree;
 }
 
----------------
LangRef also needs to be updated I guess.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1419
+    Argument *Arg = getAssociatedArgument();
+    Function *F = Arg->getParent();
+
----------------
`F = getIRPosition().getAnchorScope()`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1422
+    if (!Arg)
+      return indicatePessimisticFixpoint();
+
----------------
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.).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1430
+    const auto *TLI =
+        A.getInfoCache().getTargetLibraryInfoForFunction(*(Arg->getParent()));
+
----------------
`(Arg->getParent())` -> `F`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1450
+
+      if(LivenessAA.isAssumedDead(dyn_cast<Instruction>(UserI)))
+        continue;
----------------
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).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1456
+        if (!CB->isArgOperand(U))
+          continue;
+        if (isFreeCall(UserI, TLI))
----------------
you need to treat uses in the operand bundle as "freeing", so, `isCalleeOperand` -> ok, `isArgOperand` -> test further, if neither -> not nofree.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1458
+        if (isFreeCall(UserI, TLI))
+          return indicatePessimisticFixpoint();
+
----------------
no need for this check


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1546
+  AANoFreeCallSiteReturned(const IRPosition &IRP) : AANoFreeFloating(IRP) {}
+
+  /// See AbstractAttribute::trackStatistics()
----------------
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`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4435
 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)
----------------
leftover


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67886/new/

https://reviews.llvm.org/D67886