[PATCH] D128914: [HIP] Add support for handling HIP in the linker wrapper

Mon Jul 11 12:36:55 PDT 2022

jhuber6 added a comment.

In D128914#3642869 <https://reviews.llvm.org/D128914#3642869>, @yaxunl wrote:

> In D128914#3642567 <https://reviews.llvm.org/D128914#3642567>, @jhuber6 wrote:
>
>> In D128914#3642558 <https://reviews.llvm.org/D128914#3642558>, @JonChesterfield wrote:
>>
>>> Code looks good to me. It's hard to be sure whether it works without running a bunch of hip test cases through it, have you already done so? If it doesn't work out of the box it should be close enough to fix up post commit, e.g. when trying to move hip over to this by default.
>>
>> Thanks for the review, I ran a couple mini-apps with HIP versions (XSBench, RSBench, SU3Bench) using this method and they passed without issue. The only thing I was unsure about what whether or not the handle needed to be checked for null, because my testing suggested it's unnecessary. I was hoping one of the HIP developers would let me know. We can think about making this the default approach when I make the new driver work for `non-rdc` mode compilations.
>
> There is only one fatbin for -fgpu-rdc mode but the fatbin unregister function is called multiple times in each TU. HIP runtime expects each fatbin is unregistered only once. The old embedding scheme introduced a weak symbol to track whether the fabin has been unregistered and to make sure it is only unregistered once.

I see, this wrapping will only happen in RDC-mode so it's probably safe to ignore here? When I support non-RDC mode in the new driver it will most likely rely on the old code generation. Although it's entirely feasible to make RDC-mode the default. There's no runtime overhead when using LTO.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128914/new/

https://reviews.llvm.org/D128914