clang does not add !fpmath data to OpenCL sqrt calls

[arsenm]

By default (i.e. without -cl-fp32-correctly-rounded-divide-sqrt), calls to float/vector of float sqrt should be annotated with !fpmath metadata. Currently they are not which is interfering with fixing the library from using a global option corresponding to the flag.

I tried working towards this in bac2a07, but this is of limited use for the end use case. The actual user call to the mangled function needs to be annotated (such that a backend pass can swap out the call to the intrinsic with presered metadata) so we can lower it appropriately.

I'm not sure how to really do this. I tried hacking up LANGBUILTINs for all the mangled forms, but they don't seem to be recognized as builtins at that point for interception in CGBuiltin. Another strategy I considered was adding a macro for the correctly rounded option and setting an attribute on the declarations?

[llvmbot]

@llvm/issue-subscribers-clang-codegen

[llvmbot]

@llvm/issue-subscribers-opencl

[arsenm]

Attaching the lit test sqrt-fpmath.cl.txt

[yxsamliu]

OpenCL sqrt functions are declared in clang/lib/Headers/opencl-c.h .

How about adding a macro (e.g. __CLANG_OPENCL_USE_BULTIN_SQRT) to conditionally define them as inline functions to call clang sqrt builtins. Then AMDGPU target can choose to enable them in clang driver. If all targets are able to handle the sqrt intrinsic then this macro is unnecessary.

[arsenm]

OpenCL sqrt functions are declared in clang/lib/Headers/opencl-c.h .

How about adding a macro (e.g. __CLANG_OPENCL_USE_BULTIN_SQRT) to conditionally define them as inline functions to call clang sqrt builtins. Then AMDGPU target can choose to enable them in clang driver. If all targets are able to handle the sqrt intrinsic then this macro is unnecessary.

That could work, availabile_externally inline definition with the intrinsic call but the implementation would still need to provide a definition in the linked library. I guess we could use the macro in case of alternative implementation choices

[arsenm]

On second thought the intrinsic should always work in theory. We don't need to speculatively work around buggy implementations

[arsenm]

I tried this and it doesn't work with -fdeclare-opencl-builtins: https://reviews.llvm.org/D156743

[svenvh]

I tried this and it doesn't work with -fdeclare-opencl-builtins: https://reviews.llvm.org/D156743

-fdeclare-opencl-builtins does not provide any definitions indeed. You could move the definitions into opencl-c-base.h, which is used by both opencl-c.h and -fdeclare-opencl-builtins. That's not ideal though, as we don't want to bloat that common header.

An alternative could be to take the sqrt declarations out of opencl-c.h and OpenCLBuiltins.td and instead handle them similar to how clang handles C11 atomics (see e.g. SemaChecking.cpp).

[arsenm]

I tried this and it doesn't work with -fdeclare-opencl-builtins: https://reviews.llvm.org/D156743

-fdeclare-opencl-builtins does not provide any definitions indeed. You could move the definitions into opencl-c-base.h, which is used by both opencl-c.h and -fdeclare-opencl-builtins. That's not ideal though, as we don't want to bloat that common header.

It should be only 1 function times 6 vector overloads, so I'm not too worried about bloating it. There are some plusses to having a separate inline definition compared to fixing up call sites. It maintains inlining flexibility and avoids needing to introduce a non-standard name for the two different variants. All the fast math flag and attribute handling is also free that way.

However I'm seeing weird behavior when I move just the f32 variant in there, the double and half variants all try to resolve to calling sqrt f32.

An alternative could be to take the sqrt declarations out of opencl-c.h and OpenCLBuiltins.td and instead handle them similar to how clang handles C11 atomics (see e.g. SemaChecking.cpp).

I think this has more in common with printf, which is declared in the base header

[svenvh]

However I'm seeing weird behavior when I move just the f32 variant in there, the double and half variants all try to resolve to calling sqrt f32.

The -fdeclare-opencl-builtins mechanism only kicks in when name lookup fails. You'll probably have to provide declarations/definitions for all overloads (so including declarations for half and double) if you're providing the float definitions.