[InferAddressSpaces] Support assumed addrspaces from addrspace predicates.

[hliao]

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.

[hliao]

Fix formatting.

[hliao]

Fix formatting again following clang-format.

[arsenm]

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]

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]

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

[rampitec]

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

[hliao]

In D112041#3073560, @rampitec wrote:

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.

[rampitec]

In D112041#3073637, @hliao wrote:

In D112041#3073560, @rampitec wrote:

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]

Revise following reviewers' feedback.

[hliao]

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]

Revise commit message.

[hliao]

In D112041#3073676, @rampitec wrote:

In D112041#3073637, @hliao wrote:

In D112041#3073560, @rampitec wrote:

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.

[rampitec]

In D112041#3074418, @hliao wrote:

In D112041#3073676, @rampitec wrote:

In D112041#3073637, @hliao wrote:

In D112041#3073560, @rampitec wrote:

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]

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]

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]

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

[arsenm]

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]

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.

[nikic]

I'd still like an answer to:

In D112041#3073464, @nikic wrote:

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]

In D112041#3106936, @nikic wrote:

I'd still like an answer to:

In D112041#3073464, @nikic wrote:

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]

Rebase and kindly ping for review.

[hliao]

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp

202–203

revised in the latest patch.

[hliao]

Rebase to the main branch.

[hliao]

Rebase and ping for further review.

[hliao]

Rebase and kindly PING for review.