In OpenMP 5.2 when a nohost function is called from the host, the host variant of that function is called if one is declared.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2697–2706 | It is recommended to implement such loops as functions/lauto &ambdas: auto HasHostAttr = []() {
for (const OMPDeclareVariantAttr *A :
Callee->specific_attrs<OMPDeclareVariantAttr>()) {
auto *DeclRefVariant = cast<DeclRefExpr>(A->getVariantFuncRef());
auto *VariantFD = cast<FunctionDecl>(DeclRefVariant->getDecl());
Optional<OMPDeclareTargetDeclAttr::DevTypeTy> DevTy =
OMPDeclareTargetDeclAttr::getDeviceType(
VariantFD->getMostRecentDecl());
if (!DevTy || *DevTy == OMPDeclareTargetDeclAttr::DT_Host)
return true;
}
return false;
};
if (HasHostAttr())
return; | |
| 2703 | Why the most recent decl? | |
| 2704 | Isuppose it must be DevTy && *DevTy == OMPDeclareTargetDeclAttr::DT_Host, no? | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2703 | I believe that would retrieve the full information about the function since that's where the function is defined? | |
| 2704 | Correct me if I'm wrong but if there's no device type associated with the function then it is a host function? For the second part of the condition, if there exists a DevTy associated with the function then it has to be DT_Host. | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2697–2706 | I will rewrite this. | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2704 | Ah, I missed it. Can we have just declare variant without device type? Can you add a test for this case? | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2704 | You mean when the variant function definition does not have a device type declared? That's the case that's currently tested: #pragma omp declare variant(host_function) match(device={kind(host)})
void fun() {}
void host_function() {}But I think you're right I should add another test which tests the other part of the condition when the host_function has a device type. | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2671 | Do you have a test for !DevTy case? Maybe I'm missing it. | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2671 | Yes it was the first test I added. The second one tests the second part of the condition. | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2671 | Hmm, but in this test we have *DevTy == OMPDeclareTargetDeclAttr::DT_Host, no? | |
| clang/lib/Sema/SemaOpenMP.cpp | ||
|---|---|---|
| 2671 | I added a positive check of that condition too. No error messages to check for in that case. | |
| clang/test/OpenMP/declare_target_nohost_variant_messages.cpp | ||
|---|---|---|
| 16 ↗ | (On Diff #483643) | You mean this test case? But it still has kind(host). |
| clang/test/OpenMP/declare_target_nohost_variant_messages.cpp | ||
|---|---|---|
| 16 ↗ | (On Diff #483643) | The condition checks the attribute of the host_function which in this case is host. In the test above the condition is false because the not_a_host_function has a nohost attribute. |
| clang/test/OpenMP/declare_target_nohost_variant_messages.cpp | ||
|---|---|---|
| 16 ↗ | (On Diff #483643) | It should always have host there because we are trying to fix the case where we have a nohost function that needs a host variant. |
Comment Actions
LG
| clang/test/OpenMP/declare_target_nohost_variant_messages.cpp | ||
|---|---|---|
| 16 ↗ | (On Diff #483643) | Ah, I missed that the check was for declare target attribute. |
Comment Actions
The added test fails on 32-bit platforms:
FAIL: Clang :: OpenMP/declare_target_nohost_variant_messages.cpp (10230 of 16135) ******************** TEST 'Clang :: OpenMP/declare_target_nohost_variant_messages.cpp' FAILED ******************** Script: -- : '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 -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -fopenmp-version=52 -DVERBOSE_MODE=1 -verify=omp52 -fnoopenmp-use-tls -ferror-limit 100 -fopenmp-targets=amdgcn-amd-amdhsa -o - /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/OpenMP/declare_target_nohost_variant_messages.cpp -- Exit Code: 1 Command Output (stderr): -- + : '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 -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -fopenmp-version=52 -DVERBOSE_MODE=1 -verify=omp52 -fnoopenmp-use-tls -ferror-limit 100 -fopenmp-targets=amdgcn-amd-amdhsa -o - /var/tmp/portage/sys-devel/clang-16.0.0_pre20221225/work/clang/test/OpenMP/declare_target_nohost_variant_messages.cpp error: 'error' diagnostics seen but not expected: (frontend): OpenMP target architecture 'amdgcn-amd-amdhsa' pointer size is incompatible with host 'i686-pc-linux-gnu' -- ********************
Please fix, or ideally revert, fix and then commit properly linking to the diff.
Comment Actions
Should be a simple fix, someone needs to provide --triple= in the test line. A lot of the old tests used 64-bit PowerPC but it's not really important.
Do you have a test for !DevTy case? Maybe I'm missing it.