saiislam added a comment.
In D124525#3478469 <https://reviews.llvm.org/D124525#3478469>, @yaxunl wrote:
> need a test for the generated registration code
Yes, I will add tests.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:1161
- LinkedImages.push_back(*ImageOrErr);
+ LinkedImages.emplace_back(TheArch, *ImageOrErr);
}
----------------
jhuber6 wrote:
> I'm doing something similar in D123810, I just used the existing `DeviceFile`
> because I needed the `Arch` and `Kind` fields to dispatch the appropriate
> wrapping job for CUDA / HIP.
Seems simpler. I will pull that change here.
================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:98-104
+ // struct __tgt_image_info {
+ // int32_t version;
+ // int32_t image_number;
+ // int32_t number_images;
+ // char* offload_arch;
+ // char* target_compile_opts;
+ // };
----------------
yaxunl wrote:
> I am wondering whether we should add a few more fields to make it more
> generic for all offloading languages and platforms:
>
>
> ```
> char* target_triple;
> char* offloading_kind; // openmp, hip, etc
> char* file_type; // elf, spirv, bitcode, etc
> ```
Good idea. Though I am not sure whether file_type info is being propagated in
by the linker-wrapper or not. I will check.
================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:246
IRBuilder<> Builder(BasicBlock::Create(C, "entry", Func));
+ // Create calls to __tgt_register_image_info for each image
+ auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
----------------
jhuber6 wrote:
> I'm wondering if it would be better to create a new `__tgt_bin_desc` and call
> a new `__tgt_register_lib` with it here so we don't need multiple calls here.
> Inside that new runtime function we could just widen or shrink the existing
> structs as needed. That way each device image would have this metadata
> associated with it and the target plugin can handle it as-needed.
Last time multiple vendors objected to changing `__tgt_bin_desc` and
`__tgt_device_image` structs. The reason was backward compatibility of multiple
downstream runtimes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124525/new/
https://reviews.llvm.org/D124525
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
