llvm / clangir

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

[CIR][Dialect] Emit OpenCL kernel metadata #705

Open seven-mile opened 6 days ago

seven-mile commented 6 days ago

This PR introduces a new attribute OpenCLKernelMetadataAttr to model the OpenCL kernel metadata structurally in CIR, with its corresponding implementations of CodeGen, Lowering and Translation.

The "TypeAttr":$vec_type_hint part is tricky because of the absence of the signless feature of LLVM IR, while SPIR-V requires it. According to the spec, the final LLVM IR should encode signedness with an extra i32 boolean value.

In this PR, the droping logic from CIR's TypeConverter is still used to avoid code duplication when lowering to LLVM dialect. However, the signedness is then restored (still capsuled by a CIR attribute) and dropped again in the translation into LLVM IR.

seven-mile commented 6 days ago

For LLVM metadata that are not included in the LLVM dialect, we have to design our own attributes and passthrough it in the translation according to this thread.

Threrefore, an ideal method would be lowering from CIR attribute to LLVM IR directly. But unfortunatly we cannot access CIR's TypeConverter (produced by prepareTypeConverter) when doing translation. So a workaround of restoring and dropping again is used.

seven-mile commented 5 days ago

There are three stages for vec_type_hint here: CIR -[Lowering]-> LLVM dialect -[Translation]-> LLVM IR. This is how it works currently in this PR:

The official solution to attach metadata to LLVM IR is to call llvm::Function::setMetadata by manipulating the translation interface. If we are able to deal with TypeAttr<!cir.s32i> directly in translation, we can also emit the signedness bit straightforward, without such a workaround. But the precondition means "CIR's type converter is available for LLVM translation interface", which is not true currently (affects both clang -fclangir and cir-opt). There are no available reference for such usage, I'm not sure if it's a good practice. What do you think?

When designing the attribute, I tend to make it simple in CIR and use conservative (maybe dirty) methods for translation. The flaw resides in the mechanism of LLVM dialect, overall we should not pay for it when designing CIR. So I insist not just adding an redundant signedness bit in the attribute. Another option is to attatch the extra signedness bit to the attribute only when lowering to LLVM dialect. But that requires us to duplicate the attribute (a dedicated OpenCLKernelLLVMMetadataAttr) and also overcomplicate the IR.

As for the expensive overhead of for-loop replacement, nice catch, thanks! TBH immutable-dict-based extra-attrs is not very easy to use, while the semantics of replace make it clearer and easier to review. I can apply better indexing-based (rather than search-based) mutation after we make some progress on the discussion. The actual cost of this method should be to find out the OpenCLKernelMetadataAttr then.

Usually I want to propose a best-efforts conservative approach, to improve the acceptance of potential radical changes in the future. So I'm always open to any cleaner solutions.