[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/lib/Transforms/IPO/Attributor.cpp:1108
+    if (!ToBeExploredPaths.count(Inst)) {
+      ToBeExploredPaths.insert(Inst);
+      HasNewPath = true;
----------------
jdoerfert wrote:
> ```
> if (ToBeExploredPaths.insert(Inst).second)
>  HasNewPath = true;
> ```
You do not need the count. Just insert it and check the return value if it was already there or if it was actually added.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1018
+
+    ToBeExploredPaths.insert(&(F.getEntryBlock().front()));
+    for (size_t i = 0; i < ToBeExploredPaths.size(); ++i)
----------------
For now, we should mark the entry as live. (It would not if it contained a noreturn call right now -> test case)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1077
+  /// Collection of all assumed dead BasicBlocks.
+  DenseSet<BasicBlock *> AssumedDeadBlocks;
+
----------------
Leftover. 


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1094
+    if (ICS && ((NoReturnAA && (NoReturnAA->isAssumedNoReturn())) ||
+        ICS.hasFnAttr(Attribute::NoReturn))) {
+      NoReturnCalls.insert(I);
----------------
Maybe make this two or three conditionals so it is easier to read. First exclude non call sites, then the ones marked noreturn, then the ones assumed noreturn.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1096
+      NoReturnCalls.insert(I);
+      return false;
+    }
----------------
See the comment at the declaration. Basically, whenever we explored at least one instruction we have to report that back, e.g., to remove the NoReturnCall from the set.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1123
+
+  for (auto *I : NoReturnCalls) {
+    size_t Size = ToBeExploredPaths.size();
----------------
Do not use auto here. Not needed and not clear.

The `NoReturnCalls` are updated in `explorePath` so iterating over them while updating them is bad and will explode.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1135
+      continue;
+
+    while (Size != ToBeExploredPaths.size())
----------------
Haven't we changed the "AssumedLiveSet" here already, when the size is different I mean.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1144
+    if (NoReturnCalls.count(I))
+      NoReturnCalls.erase(I);
+
----------------
Just erase, no count necessary.


================
Comment at: llvm/test/Transforms/FunctionAttrs/liveness.ll:14
+; This is just an example. For example we can put a sync call in a
+; dead block and check if it is deduced.
+
----------------
Then do it and add the appropriate check lines :), here and in the other tests. It is an indirect test but it is better than nothing. So you will also need to use the liveness AA in the nosync AA.


================
Comment at: llvm/test/Transforms/FunctionAttrs/liveness.ll:91
+  %3 = icmp sgt i32 %2, %0
+  ret i1 %3
+}
----------------
I'm not sure what undefined behavior (UB) you mean here. Generally, we need something undefined that has a side-effect, e.g., a load null, or br undef.


================
Comment at: llvm/test/Transforms/FunctionAttrs/liveness.ll:147
+cond.elseif:                                                ; preds = %2
+  call void @inf_loop()
+  %5 = icmp slt i32 %0, %1
----------------
you mean `rec()`. Also add this test again and do not call `rec` but `testX` where `X` is the number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162