[PATCH] D67886: NoFree argument attribute.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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



================
Comment at: llvm/lib/IR/Verifier.cpp:1505
   case Attribute::NoInline:
-  case Attribute::NoFree:
+  //case Attribute::NoFree:
   case Attribute::AlwaysInline:
----------------
remove it completely please


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1419
+    Argument *Arg = getAssociatedArgument();
+    Function *F = Arg->getParent();
+
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1459
+        if (NoFreeFunc.isAssumedNoFree())
+          continue;
+
----------------
This above is not necessary if you use the function of the argument first to justify nofree.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1476
+          Worklist.push_back(&U);
+    }
+    return indicateOptimisticFixpoint();
----------------
unknown users need to be treated as potential frees.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1477
+    }
+    return indicateOptimisticFixpoint();
+  }
----------------
I doubt calling `indicateOptimisticFixpoint` is correct.


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


================
Comment at: llvm/test/Transforms/FunctionAttrs/nofree-attributor.ll:270
 attributes #1 = { nounwind }
 attributes #2 = { nobuiltin nounwind }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67886