[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 a comment.

This looks close to what I was hoping for. Some comments inlined.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1024
+  /// True if path had no dead instructions.
+  bool explorePath(Attributor &A, Instruction *I);
+
----------------
I think the return value should indicate if you explored new instructions or not.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1058
+      return false;
+    if (AssumedDeadBlocks.find(BB) != AssumedDeadBlocks.end())
+      return true;
----------------
I think they have a `count` method.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1088
+bool AAIsDeadFunction::explorePath(Attributor &A, Instruction *I) {
+  BasicBlock *BB = I->getParent();
+
----------------
You don't need to look at the whole block if you want to start at `I`.
You can go from instruction to instruction with `I->getNextNode()`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1096
+      if ((NoReturnAA && (NoReturnAA->isAssumedNoReturn() ||
+                          NoReturnAA->isKnownNoReturn())) ||
+          ICS.hasFnAttr(Attribute::NoReturn)) {
----------------
known always implies assumed. So it is sufficient to check for assumed.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1108
+
+  for (unsigned i = 0; i < NumSucc; ++i) {
+    Instruction *Inst = &(Terminator->getSuccessor(i)->front());
----------------
`for (BasicBlock *SuccBB : successors(BB)) {`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1111
+    AssumedLiveBlocks.insert(Terminator->getSuccessor(i));
+    if (!is_contained(ToBeExploredPaths, Inst))
+      ToBeExploredPaths.push_back(Inst);
----------------
Keep an additional set or a `SetVector` to make this lookup cheap.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1139
+      // If it is not in live set, then it is dead.
+      AssumedDeadBlocks.insert(&BB);
+
----------------
I'm unsure if we need two sets here. I was hoping: "not in AssumedLive" means "assumed dead". 


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1170
+    }
+  }
+
----------------
Why do we need this logic. Above, you already explore exhaustively starting from old assumed `noreturn` instructions. I hoped that was enough.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1172
+
+  if (AssumedDeadBlocks.size() != 0) {
+    indicateOptimisticFixpoint();
----------------
For now, I'd just return changed/unchanged. That can be determined by the return values of `explorePath`, e.g., if we did not explore any new instruction nothing has changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162