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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

some comments, no major problem though.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:582
+  static bool isLiveRetValue(const AAIsDead *LivenessAA,
+                             const SmallPtrSetImpl<ReturnInst *> &ReturnInsts) {
+    if (!LivenessAA)
----------------
Maybe move this as a member into AAIsDead with the argument beeing a `ArrayRef<Instruction *>` so we can reuse it


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:592
+      if (!LivenessAA->isAssumedDead(RI)) {
+        // At least one RI is live, so ReturnValue is not dead.
+        IsLive = true;
----------------
return true;


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:767
+      if (!HasOverdefinedReturnedCalls)
+        Changed = ChangeStatus::CHANGED;
       HasOverdefinedReturnedCalls = true;
----------------
I'll commit these changes as a fix today.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1252
+      return true;
+
     auto *NonNullAA = A.getAAFor<AANonNull>(*this, *CS.getInstruction(), ArgNo);
----------------
Let's move this logic into the `checkForAllCallSites` call so all users benefit.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1587
+  /// See AAIsDead::isAssumed(Instruction *I).
+  bool isAssumedDead(Instruction *I) const override {
+    if(!getAssumed())
----------------
please add an assert (also above) to verify I (or BB) is in the AnchoredScope.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1611
+      return false;
+
+    // If it is not in AssumedLiveBlocks then it for sure dead.
----------------
`return getKnown() && isAssumedDead(I)`

to reduce duplication of logic.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1709
+    // At least one new path.
+    Status = ChangeStatus::CHANGED;
+
----------------
`// At least one new instruction encountered.`


================
Comment at: llvm/test/Transforms/FunctionAttrs/arg_returned.ll:751
 ; BOTH-DAG: attributes #{{[0-9]*}} = { noinline nounwind uwtable }
-; BOTH-NOT: attributes #
----------------
Please keep these and add the additional attribute sets instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65243