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

seven-mile · 2024-06-28T06:25:00Z

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 · 2024-06-28T06:30:30Z

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.

bcardosolopes

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.

I'm not sure I understand the problem here.

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.

Same here. Can you please describe how VecTypeHintAttrwith works with examples? Perhaps I might suggest a more clean solution.

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

clang/lib/CIR/CodeGen/CIRGenFunction.cpp

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

clang/lib/CIR/CodeGen/CIRGenFunction.cpp

seven-mile · 2024-06-28T18:13:41Z

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:

In CIR, the IntType carries the signedness information from source code already. e.g. TypeAttr<!cir.s32i>
In LLVM dialect, the signedness information must be dropped after the type conversion. TypeAttr<!builtin.i32> (signless)
But we immediately restore it then, we replace the type with TypeAttr<!builtin.si32> (signed)
When doing translation, we have to provide the extra boolean bit for the signedness information according to the spec. Now we can read the restored signedness information and emit it directly. setMetadata(type: i32, signedness: 1)
However, we should drop it again before converting it to the type of LLVM IR (llvm::Type*), as signed integers are not a valid type in LLVM dialect / IR. TypeAttr<!builtin.i32> (signless again)

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.

bcardosolopes

Thanks for the comprehensive explanation, I have a better understanding of the problem now. Added extra comments w.r.t. of what needs to be done next, keep in mind we should not over design for use cases we don't have, and keep this simple instead.

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

clang/test/CIR/CodeGen/OpenCL/kernel-attributes.cl

seven-mile · 2024-07-08T07:45:45Z

Updated.

jopperm

Approach LGTM!

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVMIR.cpp

clang/lib/CIR/Dialect/IR/CIRAttrs.cpp

bcardosolopes

Thanks for the changes, few more nits

clang/include/clang/CIR/Dialect/IR/CIROpenCLAttrs.td

bcardosolopes

Awesome job, thanks! LGTM

Similar to #705, this PR implements the remaining `genKernelArgMetadata()` logic. The attribute `cir.cl.kernel_arg_metadata` is also intentionally placed in the `cir.func`'s `extra_attrs` rather than `cir.func`'s standard `arg_attrs` list. Also, the metadata is stored by `Array` with proper verification on it. See the tablegen doc string for details. This is in order to * keep it side-by-side with `cl.kernel_metadata`. * still emit metadata when kernel has an *empty* arg list (see the test `kernel-arg-meatadata.cl`). * avoid horrors of repeating the long name `cir.cl.kernel_arg_metadata` for `numArgs` times. Because clangir doesn't support OpenCL built-in types and the `half` floating point type yet, their changes and test cases are not included. Corresponding missing feature flag is added.

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.

Similar to llvm#705, this PR implements the remaining `genKernelArgMetadata()` logic. The attribute `cir.cl.kernel_arg_metadata` is also intentionally placed in the `cir.func`'s `extra_attrs` rather than `cir.func`'s standard `arg_attrs` list. Also, the metadata is stored by `Array` with proper verification on it. See the tablegen doc string for details. This is in order to * keep it side-by-side with `cl.kernel_metadata`. * still emit metadata when kernel has an *empty* arg list (see the test `kernel-arg-meatadata.cl`). * avoid horrors of repeating the long name `cir.cl.kernel_arg_metadata` for `numArgs` times. Because clangir doesn't support OpenCL built-in types and the `half` floating point type yet, their changes and test cases are not included. Corresponding missing feature flag is added.