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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:856-860
     // If any updates are made, grabs its users to the worklist because
     // their address spaces can also be possibly updated.
     LLVM_DEBUG(dbgs() << "  to " << NewAS.getValue() << '\n');
     (*InferredAddrSpace)[V] = NewAS.getValue();
 
----------------
I.e. I'd move these lines into `updateAddressSpace` and change its return type to bool, as we no longer would need NewAS' value.



================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:903-905
 Optional<unsigned> InferAddressSpacesImpl::updateAddressSpace(
-    const Value &V, const ValueToAddrSpaceMapTy &InferredAddrSpace) const {
+    const Value &V, const ValueToAddrSpaceMapTy &InferredAddrSpace,
+    PredicatedAddrSpaceMapTy &PredicatedAS) const {
----------------
hliao wrote:
> tra wrote:
> > I can't say I'm happy about the way updateAddressSpace updates PredicateAS here, but delegates updates to InferredAddrSpace to the caller. I think both should be updated in one place -- either here, or in the callee.
> the difference is that, here, we assume a use of pointer could be inferred with a new addrspace. The original is on a def of value.
I'm not sure how it explains why the function returns values to update InferredAddrSpace outside of it, but updates PredicatedAS inside.

For consistency sake, I'd update InferredAddrSpace here as well and, possibly, combine both maps into a single structure as I've suggested above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112041