This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Add signbit(long double) decl
ClosedPublic

Authored by ashi1 on Dec 10 2020, 12:54 PM.

Details

Summary

Similarly to isinfinite, isinf, and isnan, a _MSC_VER version of signbit(long double) is required for MSVC headers.

Diff Detail

Event Timeline

ashi1 requested review of this revision.Dec 10 2020, 12:54 PM
ashi1 created this revision.
tra accepted this revision.Dec 10 2020, 2:58 PM
This revision is now accepted and ready to land.Dec 10 2020, 2:58 PM

Testcase?

Not too sure how to properly write a testcase for this. I've reduced the failing case, but it requires the user to copy/install the hip headers from the HIP project.

test.cpp:

#include <hip/hip_runtime.h>

Running command:

C:\hip/bin/clang -std=c++14 -fms-extensions -fms-compatibility -isystem C:\hip/include -DHIP_ROCclr=1 -x hip test.cpp -o test.exe

In file included from test.cpp:1:
In file included from C:\hip/include\hip/hip_runtime.h:60:
In file included from C:\hip/include\hip/hcc_detail/hip_runtime.h:602:
In file included from C:\hip\lib\clang\12.0.0\include/cuda_wrappers/complex:75:
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\include\complex:249:16: error:

call to 'signbit' is ambiguous
  return (_STD signbit)(_Left);

...

tra added a comment.Jan 13 2021, 12:04 PM

For CUDA we have tests in the test-suite (https://github.com/llvm/llvm-test-suite/tree/main/External/CUDA) and a handful of buildbots running them (e.g. http://lab.llvm.org:8011/#/builders/55).
AMD should probably set up some public build/test bots for HIP, too.
In this case, manual testing and rollback if something breaks is about all we can do.

I was able to shrink down the testcase, but it still requires 2019 MSVC to be installed and Windows Kits 10 at a minimum.

test.cpp:

#include <__clang_cuda_math_forward_declares.h>
#include <cuda_wrappers/complex>
int main() {}

clang cc1 command:

C:\hip\bin\clang.exe -cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-pc-windows-msvc -emit-obj -fcuda-is-device -isystem C:\hip\lib\clang\12.0.0\include -internal-isystem "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\include" -internal-isystem "C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt" -std=c++14 -fms-extensions -fms-compatibility -fms-compatibility-version=19.28.29336 -x hip test.cpp

In file included from test.cpp:3:
In file included from C:\hip\lib\clang\12.0.0\include\cuda_wrappers/complex:75:
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\include\complex:249:16: error: call to 'signbit' is ambiguous

return (_STD signbit)(_Left);
ashi1 added a comment.Jan 14 2021, 8:43 AM
In D93062#2496542, @tra wrote:

For CUDA we have tests in the test-suite (https://github.com/llvm/llvm-test-suite/tree/main/External/CUDA) and a handful of buildbots running them (e.g. http://lab.llvm.org:8011/#/builders/55).
AMD should probably set up some public build/test bots for HIP, too.
In this case, manual testing and rollback if something breaks is about all we can do.

I will work with our team and look into adding HIP to the test-suite and AMD buildbots to the public llvm testing.

For now, is it OK to merge this patch?

Let's land this fix now. It will take some time to set up the buildbot. I think we already have this issue covered in HIP directed tests, right?

tra added a comment.Jan 14 2021, 10:00 AM

Go ahead.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 10:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript