[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Matt Arsenault via Phabricator via cfe-commits
arsenm added a comment. In https://reviews.llvm.org/D51434#1218017, @yaxunl wrote: > In https://reviews.llvm.org/D51434#1217971, @arsenm wrote: > > > https://reviews.llvm.org/D51209 is the patch. I think HIP will need an > > additional patch, since I think it isn’t subclassing the amdgpu toolcha

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl added a comment. In https://reviews.llvm.org/D51434#1217971, @arsenm wrote: > https://reviews.llvm.org/D51209 is the patch. I think HIP will need an > additional patch, since I think it isn’t subclassing the amdgpu toolchain Yes since HIP has different toolchain. This does not affect ke

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Matt Arsenault via Phabricator via cfe-commits
arsenm added a comment. https://reviews.llvm.org/D51209 is the patch. I think HIP will need an additional patch, since I think it isn’t subclassing the amdgpu toolchain https://reviews.llvm.org/D51434 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Matt Arsenault via Phabricator via cfe-commits
arsenm added a comment. I have a patch to change the default visibility which I think is a better option https://reviews.llvm.org/D51434 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Scott Linder via Phabricator via cfe-commits
scott.linder added a reviewer: arsenm. scott.linder added a comment. +Matt to confirm, but our executable format is a shared object and we eventually want to support preemptible symbols through the PLT. We already generate GOT entries for globals. Currently we work around the lack of PLT suppor

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Artem Belevich via Phabricator via cfe-commits
tra added a comment. I could not find anything about PLTs in AMDGPU-ABI , nor could I find anything relevant on google. I still have no idea why PLTs are required in this case. Without that info, the problem

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl added a comment. In https://reviews.llvm.org/D51434#1217772, @tra wrote: > Could you elaborate on what exactly is the problem this patch fixes? > I don't see how internalizing the symbols connects to PLTs. My understanding > is that PLTs are used to provide stubs for symbols to be resolv

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Artem Belevich via Phabricator via cfe-commits
tra added a comment. Could you elaborate on what exactly is the problem this patch fixes? I don't see how internalizing the symbols connects to PLTs. My understanding is that PLTs are used to provide stubs for symbols to be resolved by dynamic linker at runtime. AFAICT AMD does not use shared li

[PATCH] D51434: [HIP] Add -amdgpu-internalize-symbols option to opt

2018-08-29 Thread Yaxun Liu via Phabricator via cfe-commits
yaxunl created this revision. yaxunl added a reviewer: tra. Herald added subscribers: t-tye, tpr, dstuttard, wdng, kzhuravl. AMDGPU backend needs -amdgpu-internalize-symbols option for opt to work around a limitation of no PLT support, otherwise there is compilation error at -O0. https://reviews