[CIR][Dialect] Make addrspace in pointer types to model LangAS

[seven-mile]

This PR implements the solution B as discussed in #682.

• Use the syntax cir.ptr<T> cir.ptr<T, addrspace(target<0>) cir.ptr<T, addrspace(opencl_private)>
• Add a new AddressSpaceAttr, which is used as the new type of addrspace parameter in PointerType
• AddressSpaceAttr itself takes one single int64_t $value as the parameter
• TableGen templates to generate the conversion between clang::LangAS -> int64_t $value <-> text-form CIR

[bcardosolopes]

Thanks for putting extra effort to make this nice. I think the solution can be a bit more simple and save some memory:

1. Create a new AddressSpaceAttr in CIRAttrs.td
2. AddressSpaceAttr takes a MLIR integer as input
3. In extraClassDeclaration you add the enums. Note that before applying this change you can copy from the generated methods for you current impl of LangAddrSpace. Alternatively, you could try to leave the LangAddrSpace and use the generated methods (but I think it needs some anchor to be automatically generated, so this might not be feasible). You can add additional methods to query for target specific values, etc.
4. assemblyFormat should use a custum<SpaceAddress> which relies on (3) for printing/parsing. If value >= target, you can change your printing/parsing logic, but all that is hidden under AddressSpaceAttr, users are not exposed to these details.
5. pointer type takes an optional AddressSpaceAttr. No verifier on pointer type needed.

[seven-mile]

Nice suggestions as always, thanks!

I think the solution can be a bit more simple and save some memory.

Indeed. Using two separate parameters in such a common type leads to memory inefficiency. This problem must be resolved before this PR lands.

Update: It occurred to me that the two parameters are both 32-bit. Interesting coincidence.

1. Create a new AddressSpaceAttr in CIRAttrs.td
2. AddressSpaceAttr takes a MLIR integer as input

I used to try a dedicated attribute to better abstract it. But I found that CIRTypes.td does not depend on CIRAttrs.td, however, a reversed dependency exists: IntAttr takes IntType as parameter, which means the dependency is in the header rather than the implement. I had a feeling this would be a significant change, so moved it to CIRTypes.td first. Do you have any advice to break this dependency?

5. pointer type takes an optional AddressSpaceAttr. No verifier on pointer type needed.

Yes, in my initial vision, the null attribute is meant to replace LangAddrSpace::Default (will be removed if we use one single attribute). Just to confirm this behavior here ; )

[bcardosolopes]

I used to try a dedicated attribute to better abstract it. But I found that CIRTypes.td does not depend on CIRAttrs.td, however, a reversed dependency exists: IntAttr takes IntType as parameter, which means the dependency is in the header rather than the implement. I had a feeling this would be a significant change, so moved it to CIRTypes.td first. Do you have any advice to break this dependency?

This is a nasty dep, had bit me before, we need to solve it at some point. For now you could follow the same approach I did with StructLayoutAttr, you make PointerType optional attribute on top of a plain Attribute, and on the verifier you do a dyn_cast to confirm it's a AddressSpaceAttr

[bcardosolopes]

Yes, in my initial vision, the null attribute is meant to replace LangAddrSpace::Default (will be removed if we use one single attribute). Just to confirm this behavior here ; )

I'd prefer if it's an optional attribute to the pointer type, we should keep the builder that doesn't take an address space.

[bcardosolopes]

Awesome, thanks for putting the extra effort into mapping this nicely with tablegen, way to go!

LGTM with minor comment!

[seven-mile]

Hi Bruno!

Thanks for the approval. Julian, Victor and I drafted an RFC to redesign a bit the enum cases in ClangIR and just posted it on discourse to receive more comments from various users of address spaces.

The proposal further designs unified address spaces rather than a one-to-one modeling. However, it does not actually conflict with this PR.

The AS mapping from CIR to LLVM is target-specific information. When lowering pointer types with special addrspaces, we will eventually use the "TargetLowering" library to query the integer representation of the unified addrspaces.

Before this feature is available and our discussion about the AS design is finalized, I will prefer to submit a simplified version of this PR, which only includes the null-attribute case and the target case. We will lower all non-default address spaces into a target case that contains its final integer representation. (Already done in this PR)

When all the preconditions mature, we will gradually add cases like opencl_local or gpu_shared depending on our needs.

What do you think about this plan?

[jopperm]

Implementation of the first step (everything is either default or target-specific AS number) towards the new address space modeling proposal LGTM!

[bcardosolopes]

I will prefer to submit a simplified version of this PR, which only includes the null-attribute case and the target case. We will lower all non-default address spaces into a target case that contains its final integer representation. (Already done in this PR) ... When all the preconditions mature, we will gradually add cases like opencl_local or gpu_shared depending on our needs. ... What do you think about this plan?

Looks good to me, incremental is totally fine. Good call on involving the more broad community into giving input here.

The only thing that isn't pretty clear to me is what happens if non-target values are used now:

    if (langAS != LangAS::Default)
      langAS = getLangASFromTargetAS(Context.getTargetAddressSpace(langAS));

My expectation is that we should assert for anything different from Default AND target, because it's not really supported as of latest update. Am I missing something?

[seven-mile]

My expectation is that we should assert for anything different from Default AND target, because it's not really supported as of latest update.

I surely want to do that. But without early lowering of LangAS::opencl_*, it will break the testcases for SPIR-V target, which means it's a regression in functionality of CIRGen. Does other points of view, like maintaining the capabilities of CIRGen unchanged (lower to target AS immediately) or so sound more reasonable? It's somehow NFC, but not really.

[bcardosolopes]

I surely want to do that. But without early lowering of LangAS::opencl_*, it will break the testcases for SPIR-V target, which means it's a regression in functionality of CIRGen.

We usually don't allow things to silently miscompile. I also don't see a problem with keeping the last approach you had (support all address space enums) and changing the status quo after that RFC resumes.

With the current approach in this PR, you should add the asserts and an acceptable tradeoff would be to XFAIL the SPIR-V tests until you figure out the full story.

[seven-mile]

XFAIL sounds great. But I think although we can revert to the previous state of this PR, it will unfortunately fail to pass the SPIR-V test cases as well. Having no access to target specific information in CIR lowering will block all works on OpenCL C. Let me ask @sitio-couto when will it be available😉

[bcardosolopes]

XFAIL sounds great.

This version is fine, just add the asserts and the XFAIL and we should be good to go.

[jopperm]

New changes LGTM, you just need to replace the call to the deprecated cast member function.

[seven-mile]

Thanks for pointing it out! Rebased and fixed.

[bcardosolopes]

Thanks for the fixes, few minor nits remaining.