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

Wed Nov 3 07:33:57 PDT 2021

hliao added a comment.
Herald added a subscriber: asavonic.

Sorry for the late reply.

================
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 {
----------------
tra wrote:
> 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.
PredicatedAS holds only the deduced operand AS based on the assumption or other conditions. It may or may not help to infer its user's final AS. In case the user still cannot be inferred, PredicatedAS won't be used to change the final IR anyway. It's only an intermediate state during the inferring. However, I think it's a good idea to put relevant updates into `updateAddressSpace`. That seems a little bit cleaner.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112041