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

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AssumptionCache.h:109
   /// its instructions.
-  AssumptionCache(Function &F) : F(F) {}
+  AssumptionCache(Function &F, TargetTransformInfo *TTI) : F(F), TTI(TTI) {}
 
----------------
Perhaps this should have a default of `=nullptr` and save us on spelling it out for most of the users who don't care about it.


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:154
+using PredicatedAddrSpaceMapTy =
+    DenseMap<std::pair<const Value *, const Value *>, unsigned>;
 using PostorderStackTy = llvm::SmallVector<PointerIntPair<Value *, 1, bool>, 4>;
----------------
What do the values represent? It's worth a comment as it's not obvious.

Perhaps we should consolidate both maps into a struct or class. Plumbing each map separately through all the calls gets tedious.


================
Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:196
   void inferAddressSpaces(ArrayRef<WeakTrackingVH> Postorder,
-                          ValueToAddrSpaceMapTy *InferredAddrSpace) const;
+                          ValueToAddrSpaceMapTy *InferredAddrSpace,
+                          PredicatedAddrSpaceMapTy &PredicatedAS) const;
----------------
I think this could've been a reference, too.


================
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 {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112041