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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
