[PATCH] D64162: Summary: [Attributor] Liveness analysis abstract attribute used to indicate which BasicBlocks are dead and can therefore be ignored.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:705
+struct AAIsDead : public AbstractAttribute {
+    /// An abstract interface for liveness abstract attribute.
+    AAIsDead(Value &V, InformationCache &InfoCache)
----------------
Put that as a class comment.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:709
+
+    /// See AbstractAttribute::getAttrKind()/
+    virtual Attribute::AttrKind getAttrKind() const override { return ID; }
----------------
Leftover /


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:713
+    static constexpr Attribute::AttrKind ID =
+        Attribute::AttrKind(Attribute::None + 1);
+
----------------
I think we can/should use `EndAttrKinds + X` from now on.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:716
+    /// Returns true if nounwind is assumed.
+    virtual bool isAssumedDead() const = 0;
+
----------------
You should probably take a basic block here and below with a description that says the beginning (or end) of that block is assumed dead.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1017
+    for (BasicBlock &BB : F)
+      DeadBlocks.push_back(&BB);
+  }
----------------
While I proposed this method I think we can do something smarter. We can scan the function from the entry block (put the functionality somewhere as you need it again below) and mark blocks life that are reached on a path without a assumed or known `noreturn` function call. While doing so you want to record these function calls as well so we know what to look for in the update.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1028
+    LLVMContext &Ctx = AnchoredVal.getContext();
+    Attrs.emplace_back(Attribute::get(Ctx, "assumeddead"));
+  }
----------------
We do not want an attribute. Later we might want to place `llvm_unreachables` but first we need to get the logic right.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1041
+  /// Collection of all dead BasicBlocks.
+  SmallVector<BasicBlock *, 8> DeadBlocks;
+
----------------
**Asssumed** dead!


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1050
+
+LoopInfoBase<BasicBlock, Loop> AAIsDeadFunction::getLoopInfo(Function &F) {
+  auto DT = DominatorTree();
----------------
If we want to get loop info, we should get it through the attributor/infocache. For now, I'd not do any of this "infinite loop logic".


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1081
+
+  for (auto BB = DeadBlocks.begin(); BB != DeadBlocks.end(); ++BB) {
+    for (BasicBlock *PredBB : predecessors(*BB)) {
----------------
This iteration order is expensive and not strictly necessary, in the worst case the first block has a `noreturn` call and you still make everything life.

I think you should perform a bit more analysis in the initialize (see above) and then:
- Check all calls we stopped at before because they had an assumed or known `noreturn`. If they still do, skip them.
- If they lost the assumed `noreturn` make every block reachable from the call life but stop at calls that are assumed or known `noreturn` and at already assumed life blocks. The calls that stopped the traversal go into the set that we traverse in the next update.

If I'm not mistaken, that should limit the traversal of this attribute to at most two visit of each instruction in the function.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1099
+      if (!HasNoReturn && !InfLoop)
+        DeadBlocks.erase(BB);
+
----------------
Erasing while iterating will eventually explode. Keep a second set with "now life" blocks that need to be replaced or a new "StillAssumedDead" set.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1116
+
+  indicateOptimisticFixpoint();
+  return ChangeStatus::UNCHANGED;
----------------
There is no optimistic fixpoint after a single update call because the information used above, e.g., `isAssumedNoReturn`, might change over time. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162