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

Almost there, I think the tests need to be improved (check lines as noted below). Also, do we add the entry block to the live blocks?



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:723
+  /// Returns true if nounwind is known.
+  virtual bool isKnownDead() const = 0;
 };
----------------
jdoerfert wrote:
> I think these methods need to take a basic block and/or instruction.
Improve the comments of these functions (mention BB)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1024
+  /// True if path had no dead instructions.
+  bool explorePath(Attributor &A, Instruction *I);
+
----------------
jdoerfert wrote:
> I think the return value should indicate if you explored new instructions or not.
Improve the description and make sure it is actually what happens


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1037
+    assert(getAssociatedValue() &&
+           "Attempted to manifest an attribute without associated value!");
+
----------------
I don't think you should check the second condition because the associated value is not really important here anyway.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1044
+      BasicBlock *BB = I->getParent();
+      SplitBlock(BB, I->getNextNode());
+      ReplaceInstWithInst(BB->getTerminator(), Unreachable);
----------------
We need a check with a noreturn `invoke` as it is a terminator that does not have a next node. Does this work?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1069
+    if (getKnown() != getAssumed())
+      return false;
+    if (AssumedLiveBlocks.count(BB))
----------------
if known is true, assumed is true so no need for this second conditional.
The `count` condition here and above could just be: `return !AssumedLiveBlocks.count(BB);`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1090
+    ImmutableCallSite ICS(I);
+    auto *NoReturnAA = A.getAAFor<AANoReturnFunction>(*this, *I);
+
----------------
Move the `getAAFor` into `if (ICS) {`


================
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.
+
----------------
sstefan1 wrote:
> 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.
OK.

One thing though: do not only check for the existence of an unreachable, it could be anywhere. Check the surroundings (function name, then basic block name, etc.) and use `CHECK-NEXT:` to make sure the CFG looks as expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64162