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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 13 inline comments as done.
sstefan1 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1096
+      NoReturnCalls.insert(I);
+      return false;
+    }
----------------
jdoerfert wrote:
> 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.
This is checked and removed in updateImpl. In other words, when starting point is a noreturn instruction and we found a new live instruction, that call is deleted from `NoReturnCalls` as it is not noreturn anymore.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1123
+
+  for (auto *I : NoReturnCalls) {
+    size_t Size = ToBeExploredPaths.size();
----------------
jdoerfert wrote:
> 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.
I Changed this a bit now to avoid updating while iterating.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1135
+      continue;
+
+    while (Size != ToBeExploredPaths.size())
----------------
jdoerfert wrote:
> Haven't we changed the "AssumedLiveSet" here already, when the size is different I mean.
Good point. State should already be `CHANGED` at this point.


================
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.
+
----------------
jdoerfert wrote:
> 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.
Is it ok if I do this in a separate patch, once this and `noreturn` go in? In that patch I can update all attributes to use AAIsDead and update these tests accordingly. 

This test is kind of already present in the `cond.true` block. `foo` which is not nosync is called after `noreturn` call. I'm just proposing to add `CHECK` lines and liveness usage to other attributes in separate patch.


================
Comment at: llvm/test/Transforms/FunctionAttrs/liveness.ll:91
+  %3 = icmp sgt i32 %2, %0
+  ret i1 %3
+}
----------------
jdoerfert wrote:
> 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.
This is supposed to be signed overflow. It is not called anywhere, but I agree it is not the best test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162