[PATCH] D65243: [Attributor] Using liveness in other attributes.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Almost there. I added a last set of comments, most minor.

Could you add one more test case:

internal function foo, called 2 times but one call site is dead. We want to derive something about foo from the call site.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:866
+    return false;
+  }
 };
----------------
Thanks for this generic version. Please add doxygen documentation as well.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:630
+    if (!LivenessAA->isLiveInstSet(RetInsts.begin(), RetInsts.end()))
+      return true;
+
----------------
If you want LivenessAA might not be available, check for null.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:702
+    if (!LivenessAA->isLiveInstSet(ReturnInsts.begin(), ReturnInsts.end()))
+      continue;
+
----------------
Test for nullptr.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:965
+      if (LivenessAA && LivenessAA->isAssumedDead(I))
+        continue;
       // At this point we handled all read/write effects and they are all
----------------
Comments here and elsewhere say "blocks" but check instructions.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1241
+
+  if (!A.checkForAllCallSites(F, CallSiteCheck, true, this)) {
     indicatePessimisticFixpoint();
----------------
Pass `this` as reference (as we always do in the Attributor).


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1714
 
+    Function *AnchorValue = CS.getInstruction()->getParent()->getParent();
+    auto *LivenessAA = getAAFor<AAIsDead>(*AA, *AnchorValue);
----------------
Nit: `CS->getFunction()` or `CS.getInstruction()->getFunction()`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1718
+    if (LivenessAA && LivenessAA->isAssumedDead(CS.getInstruction()))
+      return true;
+
----------------
not `return true` but `continue`! Only this call site is OK if it is dead.


================
Comment at: llvm/test/Transforms/FunctionAttrs/liveness.ll:64
+  %call = call i32 @volatile_load(i32* %ptr1)
+  ;%call = call i32 @bar()
   br label %cond.end
----------------
remove the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65243