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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think this is basically done for a first version. I left last comments. We can improve on this in a follow up



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11917
+        Changed = true;
+      }
+      return true;
----------------
For stores you need to check it's the pointer operand, assuming it would be invalid IR to change the value operand with one of a different type. You should make a test and verify if it's ok or not. If it's OK, this is OK.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11944
+  bool takeAddressSpace(uint32_t AS) {
+    int32_t Cast = static_cast<int32_t>(AS);
+    if (AssumedAddressSpace == NoAddressSpace) {
----------------
Why do we cast this? Keep one type consistently.


================
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();
----------------
tianshilei1992 wrote:
> jdoerfert wrote:
> > Why check this? What can check is if the pointer is actually an AS cast we can just "undo".
> Because the doc of the function `checkForAllUses` says:
> > Check \p Pred on all (transitive) uses of \p V.
> 
> We only want direct uses of the value here, so we check if the use is the value itself. Did I get it wrong?
> We only want direct uses of the value here, so we check if the use is the value itself. Did I get it wrong?

We are generally happy modifying any use, I'd say. Should not matter much right now anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120586