Page MenuHomePhabricator

[InferAddressSpaces] Support assumed addrspaces from addrspace predicates.
ClosedPublic

Authored by hliao on Oct 18 2021, 5:39 PM.

Details

Summary
  • CUDA cannot associate memory space with pointer types. Even though Clang could add extra attributes to specify the address space explicitly on a pointer type, it breaks the portability between Clang and NVCC.
  • This change proposes to assume the address space from a pointer from the assumption built upon target-specific address space predicates, such as __isGlobal from CUDA. E.g.,
foo(float *p) {
  __builtin_assume(__isGlobal(p));
  // From there, we could assume p is a global pointer instead of a
  // generic one.
}

This makes the code portable without introducing the implementation-specific features.

Note that NVCC starts to support __builtin_assume from version 11.

Diff Detail

Event Timeline

hliao created this revision.Oct 18 2021, 5:39 PM
hliao requested review of this revision.Oct 18 2021, 5:39 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 18 2021, 5:39 PM
hliao edited the summary of this revision. (Show Details)Oct 18 2021, 5:45 PM
hliao updated this revision to Diff 380574.Oct 18 2021, 8:27 PM

Fix formatting.

hliao updated this revision to Diff 380581.Oct 18 2021, 9:33 PM

Fix formatting again following clang-format.

bmahjour removed a subscriber: bmahjour.Oct 19 2021, 6:41 AM
arsenm added inline comments.Oct 19 2021, 11:21 AM
llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
800–801

I disagree we need to do anything here, constant isn't a real address space. It would be cleaner if we didn't have it as a backend concept at all. We really just need global plus a slightly more contexts where memory can be marked as invariant. We also don't have a reasonable way we could implement this

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
888

Does this really need the parent check? Doesn't isValidAssumeForContext understand the control flow?

tra added inline comments.Oct 19 2021, 12:07 PM
llvm/include/llvm/Analysis/AssumptionCache.h
109–110

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.

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
154

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.

202–203

I think this could've been a reference, too.

874–902

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.

nikic added a subscriber: nikic.Oct 19 2021, 12:17 PM

Is it actually necessary to thread this through AssumptionCache, given how InferAddressSpaces is the only place that looks at these assumes?

Is there anything to remove assume() call after address space is inferred? We do not need it anymore.

hliao added a comment.Oct 19 2021, 1:32 PM

Is there anything to remove assume() call after address space is inferred? We do not need it anymore.

along with a few other intrinsics, assume intrinsic is discarded in SDAG and GISel.

Is there anything to remove assume() call after address space is inferred? We do not need it anymore.

along with a few other intrinsics, assume intrinsic is discarded in SDAG and GISel.

We may want to discard these earlier for the sake of Value::hasOneUse(). These are really not needed after casts are inserted.

hliao updated this revision to Diff 380791.Oct 19 2021, 3:21 PM

Revise following reviewers' feedback.

hliao marked 4 inline comments as done.Oct 19 2021, 3:25 PM
hliao added inline comments.
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
202–203

yeah, but we should address that in another change.

874–902

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.

hliao updated this revision to Diff 380798.Oct 19 2021, 4:00 PM

Revise commit message.

Is there anything to remove assume() call after address space is inferred? We do not need it anymore.

along with a few other intrinsics, assume intrinsic is discarded in SDAG and GISel.

We may want to discard these earlier for the sake of Value::hasOneUse(). These are really not needed after casts are inserted.

That sounds reasonable but may face the limit due to the fact that we run addrspace inferring several times (if my memory is right, 3 times if the backend one is counted.) Among them, we expect pointer values are prompted from memory to register so that we could infer addrspace for them further. If we remove these assume intrinsic earlier, there would be the risk that later addrspace inferring may not be able to leverage those assumptions.
NVPTX runs that inferring just once. But after that, there would be too much optimizations at the IR level.

I checked the most hasOneUse() usage in IR passes. Most of them are not applied to pointer arithmetic. Only a few cases are applied to pointers but also have quite limited conditions. I would expect we may need to enhance them case by case if we found real cases where the extra use from assume intrinsic makes code quality worse.

Is there anything to remove assume() call after address space is inferred? We do not need it anymore.

along with a few other intrinsics, assume intrinsic is discarded in SDAG and GISel.

We may want to discard these earlier for the sake of Value::hasOneUse(). These are really not needed after casts are inserted.

That sounds reasonable but may face the limit due to the fact that we run addrspace inferring several times (if my memory is right, 3 times if the backend one is counted.) Among them, we expect pointer values are prompted from memory to register so that we could infer addrspace for them further. If we remove these assume intrinsic earlier, there would be the risk that later addrspace inferring may not be able to leverage those assumptions.
NVPTX runs that inferring just once. But after that, there would be too much optimizations at the IR level.

I checked the most hasOneUse() usage in IR passes. Most of them are not applied to pointer arithmetic. Only a few cases are applied to pointers but also have quite limited conditions. I would expect we may need to enhance them case by case if we found real cases where the extra use from assume intrinsic makes code quality worse.

Thanks, that sounds reasonable. If we start seeing problems because of this we can always remove these later. Conceptually LGTM.

tra added inline comments.Oct 20 2021, 12:10 PM
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
851–852

I.e. I'd move these lines into updateAddressSpace and change its return type to bool, as we no longer would need NewAS' value.

874–902

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.

hliao added a comment.Nov 3 2021, 7:33 AM

Sorry for the late reply.

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
874–902

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.

hliao updated this revision to Diff 384433.Nov 3 2021, 7:46 AM

Move the updating action into updateAddrSpace and return a boolean true if it's really updated.

arsenm added inline comments.Nov 3 2021, 8:14 AM
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
240

The pass is already using UninitializedAddressSpace as a sentinal value; just use that instead of Optional<unsigned>?

957

*AS instead of getValue

tra added a comment.Nov 3 2021, 10:37 AM

LGTM in general, modulo remaining nits.

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
202–203

You're already changing function signatures.It would be reasonable to incorporate this cleanup, too.

hliao updated this revision to Diff 384535.Nov 3 2021, 11:29 AM

Updated.

nikic added a comment.Nov 3 2021, 11:33 AM

I'd still like an answer to:

Is it actually necessary to thread this through AssumptionCache, given how InferAddressSpaces is the only place that looks at these assumes?

I'd prefer not to infect AC with TTI if not necessary.

hliao added a comment.Nov 3 2021, 11:40 AM

I'd still like an answer to:

Is it actually necessary to thread this through AssumptionCache, given how InferAddressSpaces is the only place that looks at these assumes?

I'd prefer not to infect AC with TTI if not necessary.

We need assume intrinsic to preserve those predicates through optimizations as infer-addrespace may run several times. With assume used the same way, we probably just duplicate the logic in AC if we want to track them inside this pass. That's not a good practice though.

hliao updated this revision to Diff 384753.Nov 4 2021, 7:48 AM

Rebase and kindly ping for review.

hliao marked an inline comment as done.Nov 4 2021, 7:49 AM
hliao added inline comments.
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
202–203

revised in the latest patch.

hliao updated this revision to Diff 385083.Nov 5 2021, 8:22 AM
hliao marked an inline comment as done.

Rebase to the main branch.

hliao updated this revision to Diff 385378.Sun, Nov 7, 4:43 PM

Rebase and ping for further review.

hliao updated this revision to Diff 385483.Mon, Nov 8, 6:52 AM

Rebase and kindly PING for review.

arsenm accepted this revision.Mon, Nov 8, 7:51 AM
This revision is now accepted and ready to land.Mon, Nov 8, 7:51 AM
This revision was landed with ongoing or failed builds.Mon, Nov 8, 1:52 PM
This revision was automatically updated to reflect the committed changes.