[PATCH] D66155: [Attributor] Liveness for internal functions.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1848
+
+    // Only do initial explore once.
+    if (NoReturnCalls.empty()) {
----------------
Only do the whole conditional once. Hence, add `&& AssumedLiveBlocks.empy()` to the conditional in line 1838.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1878
+        Status = ChangeStatus::CHANGED;
+      }
     }
----------------
I'm confused. When do we need this status flag here, I mean why isn't the one above sufficient. And if this one is needed, why only if we find a new `NextNoReturnI` and not once the path size changes (as before)?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2370
+  // Don't check liveness for AAIsDead.
+  if (static_cast<const AAIsDead *>(&AA) == LivenessAA)
+    return false;
----------------
This is not a valid cast.

You want `&AA::ID == &AAIsDead::ID`


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:95
 ;             define internal nonnull align 8 i8* @f1(i8* nonnull readnone align 8 %0)
-; ATTRIBUTOR: define internal nonnull i8* @f1(i8* nonnull readnone align 8 %0)
+; ATTRIBUTOR: define internal nonnull i8* @f1(i8* nonnull readnone %0)
   %2 = icmp eq i8* %0, null
----------------
Why do we loose the align?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66155