[PATCH] D112041: [InferAddressSpaces] Support assumed addrspaces from addrspace predicates.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:800-801
+  // (!is_share(p) && !is_private(p)).
+  // FIXME: Even though there is no distinguish between CONSTANT and GLOBAL in
+  // the backend, it may still benefit by telling them from each other.
+  Value *Ptr;
----------------
I disagree we need to do anything here, constant isn't a real address space. It would be cleaner if we didn't have it as a backend concept at all. We really just need global plus a slightly more contexts where memory can be marked as invariant. We also don't have a reasonable way we could implement this


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:890
+    CallInst *CI = cast<CallInst>(AssumeVH);
+    if (CI->getParent() != BB || !isValidAssumeForContext(CI, I, DT))
+      continue;
----------------
Does this really need the parent check? Doesn't isValidAssumeForContext understand the control flow?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112041