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