[CIR][CodeGen][Dialect] Use the correct alloca address space

[seven-mile]

This PR includes

• Extension of CIRDataLayout
• Earlier attachment of DataLayout attributes to the module op, to use it in the ctor of CGM
• Fix of addrspace awareness of helper methods of pointer types in the builder
• Fix of every createAlloca occurrences with addrspace from DataLayout

[seven-mile]

• Why not query DataLayout in the builder?

No suitable way to access it. The builder should be stateless, but the data layout is stored in the module op. Another approach is to cache the alloca address space in the TypeCache, which is already done for ASTAllocaAddressSpace. Even more trickily, consider to use typeCache.AllocaInt8PtrTy.getAddrSpace() directly. I don't think it's a good idea though.

Update: Julian suggested me to find out the module op by insertion point. After trying a bit, some tests (10% or so) were failing because the current block is unlinked (parentOp is null) at that time. @bcardosolopes Do you know what's going on in this situation?

• Why not also fix the occurrence in LoweringPrepare?

Let's wait for the ABI related works to get the data layout available anytime. Then we can probably have a standard way to access it from later passes including LoweringPrepare. By the way, I'm not sure which feature flag should I use here. A new one about data layout or the current addressSpaceInAlloca?

• Why no test cases?

See discussions in #671 and #672 . If there's any idea for test cases, I'm very willing to add it.

• Where is the logic of addrspace casting?

This is a cyclic dependency. It is the direct dependency of #671, we can only test these paths under the SPIR-V target. Therefore I prefer to defer this part to #671 . You can preview the changes here.

[jopperm]

• Why no test cases?

See discussions in #671 and #672 . If there's any idea for test cases, I'm very willing to add it.

The problem here is that we'd like to test that the data layout's alloca address space is reflected in the AllocaOp's pointer types, but we don't support a target yet for which that AS is != 0 (and hence would be distinguishable in the generated IR).

[bcardosolopes]

@bcardosolopes Do you know what's going on in this situation?

The only reliable way to getParentOf is through something you are 100% sure is an operation, if you are coming from a mlir::Value, the defining op is null in case it's a block argument.

Let's wait for the ABI related works to get the data layout available anytime. Then we can probably have a standard way to access it from later passes including LoweringPrepare. By the way, I'm not sure which feature flag should I use here. A new one about data layout or the current addressSpaceInAlloca?

This seems good. I don't think lowering prepare should be involved here (at least for the context of this PR).

• Why no test cases?

See discussions in #671 and #672 . If there's any idea for test cases, I'm very willing to add it.

Please rename the PR with "[NFC]" on it then.

I was looking again into the PR where you introduced the address space to pointers, and I think the address space should be optional instead of making the assumption that the default one in "0". This would alleviate some of the assumptions we need to make in general - note that in the source language this is done through an attribute, and we probably don't want to over tag things that were never written in source code.

From @jopperm comments:

The problem here is that we'd like to test that the data layout's alloca address space is reflected in the AllocaOp's pointer types, but we don't support a target yet for which that AS is != 0 (and hence would be distinguishable in the generated IR).

I believe optional address space should help with this distinction. Additionally, if we forget about OpenCL, most of regular C/C++ doesn't care about address space, and we can spare of not having to tie up an integer attribute for something we don't care about.

Before I land this PR, I'd like us to solve this discussion.

Where is the logic of addrspace casting?

This is a cyclic dependency. It is the direct dependency of #671, we can only test these paths under the SPIR-V target. Therefore I prefer to defer this part to #671 . You can preview the changes here.

It's fine if that comes later, but keep in mind that if it's blocking you for longer it might be a good idea to sync with other people and maybe make it happen yourself.

[seven-mile]

Good idea! Thanks for helping me out. The key insight here is to accept the abstraction from the frontend partially. Let me phrase it more precisely as my understanding: Distinguish LangAS::Default from LangAS::FirstTargetAddressSpace in CIR, by letting the former become a pointer type without addrspace attribute.

Further discussion:

• Abstraction shift This solution should work flawlessly. But I still doubt that it's capturing the semantic of address space qualifiers. Is it expected to emit the address space casting from cir.ptr<T> to cir.ptr<T, addrspace(0)>? When creating alloca operations for SPIR-V, it will happen. And from the point of view of the final CIR, we can also state that this strategy is just adding an additional mark called addrspace(0) for LangAS::FirstTargetAddressSpace. Equivalent but sounds much more ad-hoc.
• Possibility to use enum as representation IMHO This does not make that much sense, because the frontend already contains abstraction leak (TargetAS in LangAS). We can hardly do better as its consumer.

[bcardosolopes]

But I still doubt that it's capturing the semantic of address space qualifiers. Is it expected to emit the address space casting from cir.ptr<T> to cir.ptr<T, addrspace(0)>? When creating alloca operations for SPIR-V, it will happen.

If we encode default as non-present than this type of cast should be allowed.

And from the point of view of the final CIR, we can also state that this strategy is just adding an additional mark called addrspace(0) for LangAS::FirstTargetAddressSpace.

Well, it's not like you have proposed a non-adhoc solution yet. I'm trying to poke you for proposing alternatives, but all you seem to be making is assumptions instead of proposing some design.

Equivalent but sounds much more ad-hoc.

Still better than current proposed.

IMHO This does not make that much sense, because the frontend already contains abstraction leak (TargetAS in LangAS). We can hardly do better as its consumer.

This is ClangIR, not GenericIR, if we need to encode Clang specific information, and that makes sense for our representation, that's completely reasonable.

Take a step back and look at the definitions:

enum class LangAS : unsigned {
  // The default value 0 is the value used in QualType for the situation
  // where there is no address space qualifier.
  Default = 0,

  ...
  opencl_global,
  ...
  // Wasm specific address spaces.
  wasm_funcref,

  // This denotes the count of language-specific address spaces and also
  // the offset added to the target-specific address spaces, which are usually
  // specified by address space attributes __attribute__(address_space(n))).
  FirstTargetAddressSpace
};

Few more suggestions:

Solution A

Number based: Default -> "no address space qualifier" opencl_global, ... -> "address_space(value_of_enum)" FirstTargetAddressSpace / __attribute__(address_space(0))) -> "address_space(0)" FirstTargetAddressSpace+1 / __attribute__(address_space(1))) -> "address_space(1)" (this might clash with existing enums) ...

Solution B

Enum based: { opencl_global, opencl_global_device, ... target_specific }

Default -> "no address space qualifier" opencl_global -> "address_space(opencl_global)" opencl_global_device -> "address_space(opencl_global_device)" ... __attribute__((address_space(0))) -> "address_space(target<0>)" __attribute__((address_space(1))) -> "address_space(target<1>)" ...

For this, address_space attribute would need (a) enum kind, (b) optional value (for target enum). Extra thoughts? Suggestions?

[bcardosolopes]

Let me know when this is ready for another round of review, thanks!