[PATCH] D62687: [Attributor] Deduce "nofree" function attribute
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 30 09:55:36 PDT 2019
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
More information about the llvm-commits
mailing list