[PATCH] D62687: [Attributor] Deduce "nofree" function attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a subscriber: hfinkel.
jdoerfert added a comment.

Please make this depend on the no-free patch by @hfinkel. We should also see if we extract the definition from there or if he plans to work on that patch further.

Some comments inlined.

I don't know why we would need TLI here but could you split the addition of it so we have it for the future.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:187
+    auto StringAttr = Attr.getKindAsString();
+    if (StringAttr == "nofree") {
+      NumFnNoFree++;
----------------
We want an llvm::StringSwitch here.

We could also move the whole bookkeeping into the AbstractAttributes. That might actually be nicer.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:724
+      : AbstractAttribute(F, InfoCache), TLI(TLI) {}
+
+  /// See AbstractAttribute::getState()
----------------
Please add `bool isAssumedNoFree()` and `bool isKnownNoFree()`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:759
+ChangeStatus AANoFreeFunction::updateImpl(Attributor &A) {
+    Function &F = cast<Function>(getAnchoredValue());
+
----------------
You can directly get the AnchorScope


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:771
+          return ChangeStatus::CHANGED;
+        }
+        if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
----------------
I don't see why we'd need this conditional (and the Realloc check).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:800
+        ImmutableCallSite ICS(I);
+        assert(ICS);
+        if (!ICS.hasFnAttr("nofree")) {
----------------
No need for the assert, and if you want one, add a message.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:801
+        assert(ICS);
+        if (!ICS.hasFnAttr("nofree")) {
+          auto *NoFreeAA = A.getAAFor<AANoFreeFunction>(*this, *I);
----------------
You can make safe indention if you continue for `nofree` call sites. Though, I don't see why we should check the attribute in the first place. The following check is what we want.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:803
+          auto *NoFreeAA = A.getAAFor<AANoFreeFunction>(*this, *I);
+          if (!NoFreeAA || !NoFreeAA->isValidState()) {
+            indicatePessimisticFixpoint();
----------------
Please add `|| !NoFreeAA->isAssumedNoFree()` for concistency.

Revert the condition for less indented code.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:810
+    }
+    indicateOptimisticFixpoint();
+    return ChangeStatus::UNCHANGED;
----------------
There is no fixpoint reached here, at least not if `getAAFor` was called at least once! Just remove the line and let the framework determine this for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62687