search

llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
307 stars 84 forks source link

[CIR][Dialect] Alloca address space verification #672

Closed seven-mile closed 3 weeks ago

seven-mile commented 3 weeks ago

This PR ensures the result address space of alloca matches the expectation of data layout.

seven-mile commented 3 weeks ago

Inspired by this commit.

It's useful because builder.createAlloca in CIR does not consider address spaces, but CreateAlloca in LLVM IR Builder does.

https://github.com/llvm/clangir/blob/9720c614b93406c6a08a04887958c2e0e3ad1632/llvm/include/llvm/IR/IRBuilder.h#L1773-L1786

There are two versions, one for free address space specification, which means this check is not compulsory. But at this early stage of address space support, we should be glad to have it.

It also makes it easier to track the absence of getASTAllocaAddressSpace in #671 , which basically returns the address space from data layout.

@orbiri What do you think? ; )

orbiri-ns commented 3 weeks ago

Very nice! Do we specify in the op description that the address space of the returned pointer must match the address space specified in the DL? Please add it if not :)

jopperm commented 3 weeks ago

It's useful because builder.createAlloca in CIR does not consider address spaces, but CreateAlloca in LLVM IR Builder does. [...] That's why it's a verifier, rather than an enhancement in CIRBuilder.

I haven't understood the rationale yet, sorry! Trying to condense the discussion above, is there anything preventing us from doing the following (as a new PR, to prepare for #671)

seven-mile commented 3 weeks ago

We could not mimic such approach immediately, because there's no actual need for "non-default (non-0) alloca address space" in CIR. (SPIR-V target does not need it.) I don't think it's a good idea to add this feature now.

I used to insist that I have no way to test the DL query in CIRGen, it shall always emit zero still. But I do have a way to test the verification (like invalid.cir in this PR). I'm assuming such enhancement with no actual need and no observable test cases could hardly be accepted, by previous experience. Therefore I stated "I don't think it's a good idea to add this feature now". These two PRs are a big workaround against it.

But of course, I'll be glad to follow the direct and perfect approach as Julian suggested, if the community prefers it more. And that's the first prototype I developed actually😂. When submitting it, I found it may not pass the review and managed to get this workaround. Sorry to take up your time, it's still a constructive discussion overall😉. Closing as not planned.