[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:5971
+  /// the associated value is dead.
+  virtual int32_t getAddressSpace() const = 0;
+
----------------
no null opt anymore


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5989
+
+  static const int32_t NoAddressSpace = -1;
+
----------------
docs


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11912-11920
+    auto *LivenessAA =
+        A.getAAFor<AAIsDead>(*this, getIRPosition(), DepClassTy::REQUIRED);
+    if (!LivenessAA->isValidState())
+      return indicatePessimisticFixpoint();
+
+    bool UsedAssumedInformation = false;
+    if (A.isAssumedDead(getIRPosition(), this, LivenessAA,
----------------
All of this happens implicitly. No need for you to check it. It would also not be required.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11924-11926
+                                                        DepClassTy::REQUIRED);
+    if (!AUO->getState().isValidState())
+      return indicatePessimisticFixpoint();
----------------
Check out other uses of AAUnderlyingObjects. Instead of requiring it, you can just look at the associated value in case the `forallUnderlyingObjects` call fails.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11934
+      AddressSpaces.insert(Obj.getType()->getPointerAddressSpace());
+      return true;
+    };
----------------
Vector is overkill. You can return false as soon as we know we have more than one value. You can directly work on the AssumedAddressSpace.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11961
+
+    SmallVector<Use *> WorkList;
+
----------------
No need for the vector, change the uses in the first loop.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11967-11970
+      auto *LivenessAA =
+          A.getAAFor<AAIsDead>(*this, InstPos, DepClassTy::REQUIRED);
+      if (A.isAssumedDead(InstPos, this, LivenessAA, UsedAssumedInformation))
+        continue;
----------------
No need. Just let the attributor change all uses.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11996
+
+  void trackStatistics() const override {}
+
----------------
add a statistic counter please


================
Comment at: llvm/test/Transforms/Attributor/address_space_info.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --check-attributes --check-globals --prefix-filecheck-ir-name true
+; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -attributor-annotate-decl-cs -attributor-max-iterations=2 -S < %s | FileCheck %s --check-prefixes=CHECK
+; RUN: opt -aa-pipeline=basic-aa -passes=attributor-cgscc -attributor-manifest-internal -attributor-annotate-decl-cs -S < %s | FileCheck %s --check-prefixes=CHECK
----------------
remove the max iterations flag


================
Comment at: llvm/test/Transforms/Attributor/address_space_info.ll:4
+; RUN: opt -aa-pipeline=basic-aa -passes=attributor-cgscc -attributor-manifest-internal -attributor-annotate-decl-cs -S < %s | FileCheck %s --check-prefixes=CHECK
+
+define float @load_global_from_flat(float* %generic_scalar) #0 {
----------------
transform this to opaque ptr please.


================
Comment at: llvm/test/Transforms/Attributor/address_space_info.ll:17
+  ret float %tmp1
+}
+
----------------
What are we testing here?


================
Comment at: llvm/test/Transforms/Attributor/address_space_info.ll:32
+  ret float %tmp1
+}
+
----------------
What are we testing here?


================
Comment at: llvm/test/Transforms/Attributor/address_space_info.ll:196
+  ret void
+}
+
----------------
Make a test where the value and pointer operand are the same and can be casted to an AS.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120586