This is an archive of the discontinued LLVM Phabricator instance.

[CUDA][HIP] Rename and fix `-fcuda-approx-transcendentals`
ClosedPublic

Authored by yaxunl on Jul 9 2023, 1:28 PM.

Details

Summary

Rename -fcuda-approx-transcendentals as
-fgpu-approx-transcendentals and pass it
to both device and host clang -cc1.

Fix its interaction with -ffast-math to allow
-fno-gpu-approx-transcendentals to override
the implicit -fcuda-approx-transcendentals
due to -ffast-math.

Rename the predefined macro to be
__CLANG_GPU_APPROX_TRANSCENDENTALS__.
Emit the macro for both device and host compilation.

Diff Detail

Event Timeline

yaxunl created this revision.Jul 9 2023, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2023, 1:28 PM
yaxunl requested review of this revision.Jul 9 2023, 1:28 PM
tra added a comment.Jul 10 2023, 9:46 AM

Looks good in general.

clang/lib/Driver/ToolChains/Clang.cpp
7251–7253
bool UseApproxTranscendentals = Args.hasFlag(options::OPT_ffast_math, options::OPT_fno_fast_math,  false));
clang/lib/Frontend/InitPreprocessor.cpp
1302–1303

We may want to rename the macro to __CLANG_GPU_APPROX_TRANSCENDENTALS__, too.

MaskRay added inline comments.Jul 10 2023, 10:51 AM
clang/test/Driver/hip-options.hip
179

excess spaces before %s

ditto below

Prefer --check-prefix= when there is one single check.

yaxunl marked 3 inline comments as done.Jul 10 2023, 1:08 PM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
7251–7253

fixed

clang/lib/Frontend/InitPreprocessor.cpp
1302–1303

will emit __CLANG_GPU_APPROX_TRANSCENDENTALS__

clang/test/Driver/hip-options.hip
179

fixed

yaxunl updated this revision to Diff 538787.Jul 10 2023, 1:10 PM
yaxunl marked 3 inline comments as done.
yaxunl edited the summary of this revision. (Show Details)

revised by comments

tra accepted this revision.Jul 10 2023, 1:30 PM
tra added inline comments.
clang/lib/Frontend/InitPreprocessor.cpp
1304

I think we can remove it. I don't think we need to keep the old one around. Internal headers have been changed and the macro was never intended for public use.

This revision is now accepted and ready to land.Jul 10 2023, 1:30 PM
MaskRay added inline comments.Jul 10 2023, 1:30 PM
clang/lib/Driver/ToolChains/Clang.cpp
7258

You can use Args.claimAllArgs(options::OPT_fgpu_approx_transcendentals, options::OPT_fno_gpu_approx_transcendentals);

clang/test/Driver/hip-options.hip
184

Just test -cc1: // APPROX: "-cc1"{{.*}} "-triple" "amdgcn-amd-amdhsa" {{.*}} "-fgpu-approx-transcendentals"

Testing clang requires -no-canonical-prefixes https://maskray.me/blog/2021-03-28-compiler-driver-and-cross-compilation#misc

209

If %t happens to be in a path with warning as a substring, this will spuriously fail.

Suggest %clang -fdriver-only -Werror... 2>&1 | count 0 to test that there is no warning/error.

MaskRay accepted this revision.Jul 10 2023, 1:30 PM

Some nits about testing

This revision was landed with ongoing or failed builds.Jul 25 2023, 9:01 AM
This revision was automatically updated to reflect the committed changes.
yaxunl marked 4 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 9:01 AM
yaxunl added inline comments.Jul 25 2023, 9:02 AM
clang/lib/Driver/ToolChains/Clang.cpp
7258

will do

clang/lib/Frontend/InitPreprocessor.cpp
1304

will remove

clang/test/Driver/hip-options.hip
209

will do