[Openmp-dev] Declare variant + Nvidia Device Offload

[Johannes Doerfert via Openmp-dev]

On 5/20/20 12:59 PM, Lukas Sommer wrote:
> On 20.05.20 19:34, Johannes Doerfert wrote:
>
>> On 5/20/20 12:26 PM, Jonas Hahnfeld wrote:
>>> Am Mittwoch, den 20.05.2020, 11:40 -0500 schrieb Johannes Doerfert:
>>>> On 5/20/20 11:19 AM, Jonas Hahnfeld wrote:
>>>>> Am Dienstag, den 19.05.2020, 17:50 +0200 schrieb Lukas Sommer via
>>>>> Openmp-dev:
>>>>>> Hi Johannes,
>>>>>>
>>>>>>> Yes, we need to replace the dot with either of the symbols you
>>>>>>> mentioned. If we can use the same symbols on the host, I'm fine with
>>>>>>> chaining it unconditionally.
>>>>>> I've prototyped this change locally, replacing the dot with a
>>>>>> dollar-sign in the places you mentioned. With this change, declaring
>>>>>> variants for offloading to CUDA devices works for my testcase in my
>>>>>> setup (ARM + Nvidia SM 7.2).
>>>>>>
>>>>>> If you want, I could create a patch for that, but do you (or
>>>>>> anyone else
>>>>>> on the list) know whether the other offloading targets (AMDGPU,
>>>>>> NEC VE,
>>>>>> ...) are all able to handle the dollar sign in the mangled name,
>>>>>> so we
>>>>>> could make this change unconditionally? I do not have a setup for
>>>>>> each
>>>>>> of these to test.
>>>>>>
>>>>>> I was also wondering if legalizing the mangled name shouldn't be
>>>>>> handled
>>>>>> by the backend (NVPTX in this case) instead of the OpenMP-specific
>>>>>> parts.
>>>>> For some context on this, see the following set of reviews:
>>>>> https://reviews.llvm.org/D17738
>>>>> https://reviews.llvm.org/D29883
>>>>> https://reviews.llvm.org/D39005
>>>>> https://reviews.llvm.org/D40573
>>>>>
>>>>> My favorite statement from the last patch: "This is silly. This bug
>>>>> has
>>>>> been open for so long that nvidia could've just fixed their toolchain
>>>>> by now to accept dots in symbol names."
>>>> Thanks for the links!
>>>>
>>>> I think in those reviews people also concluded you cannot/should not
>>>> modify visible symbol names late.
>>>>
>>>> I guess, since we have _$_ in other cases we should do _$_ as separator
>>>> in Clang too. WDYT?
>>> Probably depending on the target architecture, see for example
>>> CGOpenMPRuntime::getOutlinedHelperName() and the override in
>>> CGOpenMPRuntimeNVPTX for precedence.
>>
>> I would like to avoid using just __, for c++filt reasons.
>> CGOpenMPRuntime is
>>
>> going away, if we want to change something this is the time.
>>
> IMHO, if we can have a single solution that works on all (currently
> supported) offloading targets, we should use that and avoid additional
> complexity of a target-dependent solution.
>
> _$_ seems like a reasonable solution, as ISO C/C++ forbids dollar-signs
> in identifiers (even though most compilers still accept it/have flags
> for it.)
>
> WDYT? Would you prefer a mechanism for a target-dependent separator?


I'd really prefer something that works with c++filt if we don't have

a reason not to. If you pick a separator starting with "$" it should

work fine. If there is an underscore or not after, I don't care much.


>> Please prepare a patch. People can then test it, e.g., AMD folks, but I
>> expect it to work fine and us adopting it
> Ok, will do after we agree on a solution.


I CC'ed Ravi and Jon to confirm that Intel and AMD toolchains will be

be fine with "$" in the symbol name. We already know it should work for

NVIDIA and NEC. I guess that would be all.




> Best,
>
> Lukas
>
>