This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][AMDGPU] Enable use of abs labs and llabs math functions in C code
ClosedPublic

Authored by doru1004 on Dec 9 2022, 9:07 AM.

Details

Summary

Enable use of abs labs and llabs math functions in C code. Before this patch these math functions could only be used in OpenMP target regions in C++.

Diff Detail

Event Timeline

doru1004 created this revision.Dec 9 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 9:07 AM
doru1004 requested review of this revision.Dec 9 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
doru1004 retitled this revision from [OpenMP][AMDGPU] Enable use of abs labs and llabs function in C code to [OpenMP][AMDGPU] Enable use of abs labs and llabs math functions in C code.Dec 9 2022, 9:07 AM

I'm not 100% sure if this was excluded on purpose or not. FWIW, in C, these functions are not defined in math.h (https://en.cppreference.com/w/c/numeric/math/abs), but in C++ they are (https://en.cppreference.com/w/cpp/numeric/math/abs).

Once we enable libm_gpu it might not make a difference. For now, I'm OK with this if it doesn't break peoples code.

I'm not 100% sure if this was excluded on purpose or not. FWIW, in C, these functions are not defined in math.h (https://en.cppreference.com/w/c/numeric/math/abs), but in C++ they are (https://en.cppreference.com/w/cpp/numeric/math/abs).

Once we enable libm_gpu it might not make a difference. For now, I'm OK with this if it doesn't break peoples code.

Should there be a separate builtin header corresponding to stdlib.h?

I'm not 100% sure if this was excluded on purpose or not. FWIW, in C, these functions are not defined in math.h (https://en.cppreference.com/w/c/numeric/math/abs), but in C++ they are (https://en.cppreference.com/w/cpp/numeric/math/abs).

Once we enable libm_gpu it might not make a difference. For now, I'm OK with this if it doesn't break peoples code.

Should there be a separate builtin header corresponding to stdlib.h?

Probably. And we want to finish the libc_gpu/libm_gpu work to provide the definitions late (in LTO mode)

doru1004 updated this revision to Diff 481722.Dec 9 2022, 12:16 PM

I'm not 100% sure if this was excluded on purpose or not. FWIW, in C, these functions are not defined in math.h (https://en.cppreference.com/w/c/numeric/math/abs), but in C++ they are (https://en.cppreference.com/w/cpp/numeric/math/abs).

Once we enable libm_gpu it might not make a difference. For now, I'm OK with this if it doesn't break peoples code.

This fixes the case where we have a C file with offloading OpenMP in it that calls these functions. I'm not sure I see another way to do this in the current setup but I'm open to other solutions if this one breaks things unexpectedly.

doru1004 updated this revision to Diff 481738.Dec 9 2022, 1:36 PM
arsenm added inline comments.Dec 9 2022, 1:38 PM
clang/lib/Headers/__clang_hip_math.h
145 ↗(On Diff #481738)

Why not introduce __clang_hip_stdlib.h now?

doru1004 added inline comments.Dec 9 2022, 2:34 PM
clang/lib/Headers/__clang_hip_math.h
145 ↗(On Diff #481738)

Do we want to do this in this patch or as a separate patch? Is the work on libc_gpu/libm_gpu going to build up on these headers or will these headers be replaced in the process?

arsenm added inline comments.Dec 9 2022, 2:55 PM
clang/lib/Headers/__clang_hip_math.h
145 ↗(On Diff #481738)

Yes. I think this kind of thing is hard to untangle once people start using it

doru1004 updated this revision to Diff 482133.Dec 12 2022, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 8:06 AM

@arsenm I have factored out the abs/labs/llabs functions in a separate __clang_hip_stdlib.h file which could be included by C sources.

arsenm accepted this revision.Dec 13 2022, 2:05 PM
This revision is now accepted and ready to land.Dec 13 2022, 2:05 PM
doru1004 updated this revision to Diff 483312.Dec 15 2022, 1:03 PM
arsenm accepted this revision.Dec 15 2022, 4:40 PM
doru1004 updated this revision to Diff 483659.Dec 16 2022, 1:43 PM
mgorny reopened this revision.Dec 25 2022, 5:42 AM
mgorny added a subscriber: mgorny.

The added test fails on 32-bit platforms:

******************** TEST 'Clang :: Headers/amdgcn_openmp_device_math_c.c' FAILED ********************
Script:
--
: 'RUN: at line 2';   /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/bin/clang -cc1 -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/bin/../../../../lib/clang/16/include -nostdsysteminc -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/Inputs/include -x c -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -D__OFFLOAD_ARCH_gfx90a__ -emit-llvm-bc /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/amdgcn_openmp_device_math_c.c -o /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/test/Headers/Output/amdgcn_openmp_device_math_c.c.tmp-host.bc
: 'RUN: at line 3';   /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/bin/clang -cc1 -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/bin/../../../../lib/clang/16/include -nostdsysteminc -include __clang_hip_runtime_wrapper.h -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/../../lib/Headers/openmp_wrappers -include __clang_openmp_device_functions.h -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/../../lib/Headers/openmp_wrappers -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/Inputs/include -x c -fopenmp -triple amdgcn-amd-amdhsa -aux-triple x86_64-unknown-unknown -fopenmp-targets=amdgcn-amd-amdhsa -emit-llvm /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/amdgcn_openmp_device_math_c.c -fopenmp-is-device -fopenmp-host-ir-file-path /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/test/Headers/Output/amdgcn_openmp_device_math_c.c.tmp-host.bc -o - | /usr/lib/llvm/16/bin/FileCheck /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/amdgcn_openmp_device_math_c.c --check-prefixes=CHECK
--
Exit Code: 1

Command Output (stderr):
--
+ : 'RUN: at line 2'
+ /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/bin/clang -cc1 -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/bin/../../../../lib/clang/16/include -nostdsysteminc -internal-isystem /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/Inputs/include -x c -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -D__OFFLOAD_ARCH_gfx90a__ -emit-llvm-bc /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/Headers/amdgcn_openmp_device_math_c.c -o /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/x/y/clang-abi_x86_32.x86/test/Headers/Output/amdgcn_openmp_device_math_c.c.tmp-host.bc
error: OpenMP target architecture 'amdgcn-amd-amdhsa' pointer size is incompatible with host 'i686-pc-linux-gnu'

--

********************

Please fix or revert.

This revision is now accepted and ready to land.Dec 25 2022, 5:42 AM

Also, please link commits to diffs using Differential Revision: trailer in the future.

mgorny closed this revision.Dec 26 2022, 1:39 AM

I've pushed a fix in dab67c66932b9149842f7c8431e951f952125fc0, based on @jhuber6's fix from the other diff.

I've pushed a fix in dab67c66932b9149842f7c8431e951f952125fc0, based on @jhuber6's fix from the other diff.

Thank you for pushing a fix! :)