[PATCH] D64162: [Attributor] Liveness analysis.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This looks generally fine to me. Some small comments that are easy to fix but on bigger request wrt. Invokes. After thinking about this I realized it is not as simple, and the code that already deals with this proved me right. We should reuse that logic to replace Invokes, or even better, to insert unreachables where possible. Take a look at the inlined comments and let me know if you encounter problems.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:789
+/// An AbstractAttribute for noreturn.
+struct AANoReturn : public AbstractAttribute, BooleanState {
+
----------------
Remove the `BooleanState` part.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:819
+  static constexpr Attribute::AttrKind ID =
+      Attribute::AttrKind(Attribute::EndAttrKinds + 1);
+
----------------
Just before you commit, make sure this is unique, thus no other attribute we comited in the meantime has `EndAttrKinds + 1` as ID. I will come up with a "better" system soon.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1364
+        CallInst *CI = CallInst::Create(F->getFunctionType(), F);
+        ReplaceInstWithInst(II, CI);
+        CI->getParent()->getInstList().push_back(Unreachable);
----------------
Please make `changeToCall(..)` in `llvm/lib/Transforms/Utils/Local.cpp` visible to the outside (also rename it properly) and then use it. We do want to avoid duplication and changing invoke to call is not as simple as this.
We could even go further and expose the "insert unreachable" logic that is present in the `Local.cpp` such that you could call it here for each `I`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1389
+      // return false;
+    // return true;
+  }
----------------
Get rid of these comments, also below.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1409
+  /// Collection of calls with noreturn attribute, assumed or knwon.
+  DenseSet<Instruction *> NoReturnCalls;
+};
----------------
Make this a `(Small)SetVector` to ensure deterministic exploration. It is not strictly necessary for correctness but confusing if we get different results on a timeout. It also makes it harder to debug.

Also consider making the three members above private.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162