[PATCH] D120586: [Attributor] Add AAAddressSpaceInfo to deduce address spaces

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5981
+      : StateWrapper<BooleanState, AbstractAttribute>(IRP) {}
+
+  /// Return the address space of the associated value. \p NoAddressSpace is
----------------
Checkout `isValidIRPositionForInit` in upstream attributes. Use it to restrict this one.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11875
+           "Associated value is not a pointer");
+    A.getAAFor<AAIsDead>(*this, getIRPosition(), DepClassTy::OPTIONAL);
+  }
----------------
No need for AAIsDead.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11881-11884
+    if (!AUO->getState().isValidState())
+      return takeAddressSpace(getAssociatedType()->getPointerAddressSpace())
+                 ? ChangeStatus::CHANGED
+                 : ChangeStatus::UNCHANGED;
----------------
No need. Most attributes behave correct w/o state check. This one does too.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11892-11898
+      if (AssumedAddressSpace == NoAddressSpace) {
+        (void)takeAddressSpace(AS);
+        return true;
+      }
+      assert(AssumedAddressSpace >= 0 &&
+             "AssumedAddressSpace should not be negative");
+      return AS == static_cast<uint32_t>(AssumedAddressSpace);
----------------
Do all of this is takeAS, no need to have the logic split. It should return true if we are still "valid", and false otherwise. You can just do `return takeAS(AS)`


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11914
+            getAssociatedType()->getPointerAddressSpace())
+      return ChangeStatus::UNCHANGED;
+
----------------
Technically, you can replace the value with poison if you have NoAS at the end.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11923-11924
+    auto Pred = [&](const Use &U, bool &) {
+      if (U.get() != AssociatedValue)
+        return true;
+      User *Usr = U.getUser();
----------------
Why check this? What can check is if the pointer is actually an AS cast we can just "undo".


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:12004
+  ChangeStatus updateImpl(Attributor &A) override {
+    return ChangeStatus::UNCHANGED;
+  };
----------------
or put this into the initialize of all the ones we do not actually support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120586