This is an archive of the discontinued LLVM Phabricator instance.

Differential D57716

[CUDA][HIP] Check calling convention based on function target
ClosedPublic

Authored by yaxunl on Feb 4 2019, 2:35 PM.

Details

Reviewers
rjmccall
tra
Commits
rG785cbd850b76: [NFC] minor revision of r354929 [CUDA][HIP] Check calling convention based on…
rL354990: [NFC] minor revision of r354929 [CUDA][HIP] Check calling convention based on…
rC354990: [NFC] minor revision of r354929 [CUDA][HIP] Check calling convention based on…
rGfa49c3a888e8: [CUDA][HIP] Check calling convention based on function target
rL354929: [CUDA][HIP] Check calling convention based on function target
rC354929: [CUDA][HIP] Check calling convention based on function target
Summary

MSVC header files using vectorcall to differentiate overloaded functions, which
causes failure for AMDGPU target. This is because clang does not check function
calling convention based on function target.

This patch checks calling convention using the proper target info.

Diff Detail

Event Timeline

yaxunl created this revision.Feb 4 2019, 2:35 PM
Herald added subscribers: t-tye, tpr, dstuttard and 4 others. · View Herald TranscriptFeb 4 2019, 2:35 PM
yaxunl updated this revision to Diff 185375.Feb 5 2019, 12:24 PM
yaxunl retitled this revision from Let AMDGPU compile MSVC headers containing vectorcall to [CUDA][HIP] Check calling convention based on function target.
yaxunl edited the summary of this revision. (Show Details)
yaxunl added a reviewer: tra.

My last fix is not right. This patch fixes the issue by checking calling convention based on whether it is host or device function.

hliao added a subscriber: hliao.Feb 5 2019, 12:30 PM
tra added inline comments.Feb 5 2019, 1:44 PM
lib/Sema/SemaDeclAttr.cpp
4621–4638

This is rather convoluted.
Perhaps it would be easier to understand if the code is restructured. along these lines:

// Returns a tuple indicating whether we need to check TargetInfo on host/device side of the compilation
bool CheckHost = false, CheckDevice=false;

switch (CudaTarget) { 
    case CFT_HostDevice: 
      CheckHost = true; CheckDevice=true; break;
    case CFT_Host: 
      CheckHost = true; break;
    case CFG_Device: 
    case CFG_Global: 
      CheckDevice=true; break;
  }
} ();
TargetInfo *HostTI = CudaIsDevice ? Aux : &TI;
TargetInfo * DeviceTI = CudaIsDevice ? &TI : Aux;
if (CheckHost && HostTI)
  HostTI->checkCallingConvention(CC);
if (CheckDevice && DeviceTI)
    DeviceTI->checkCallingConvention(CC);
test/SemaCUDA/amdgpu-windows-vectorcall.cu
3

It may be good to add a check where we *would* expect to see the diagnostics.

yaxunl updated this revision to Diff 187862.Feb 21 2019, 2:04 PM

Revised by Artem's comments.

Herald added a subscriber: jdoerfert. · View Herald TranscriptFeb 21 2019, 2:04 PM
tra added inline comments.Feb 21 2019, 2:25 PM
test/SemaCUDA/amdgpu-windows-vectorcall.cu
3

It may be good to add a check where we *would* expect to see the diagnostics.

Scratch that. Wrong calling convention attribute will be ignored.

10–11

I don't think we need templates here. We're only making sure that the function attributes are handled correctly.
Can we get by with a regular function declaration or definition?

yaxunl marked an inline comment as done.Feb 22 2019, 8:40 AM
yaxunl added inline comments.
test/SemaCUDA/amdgpu-windows-vectorcall.cu
10–11

Here we want to make sure _Ret (__cdecl _Arg0::*)(_Types) and _Ret (__vectorcall _Arg0::*)(_Types) are different types because calling convention is part of type, otherwise it results in compilation error due to duplicate template instantation. I can come up with a test without template.

yaxunl updated this revision to Diff 187943.Feb 22 2019, 8:43 AM

modify test to use non-template functions.

yaxunl added a comment.Feb 26 2019, 8:23 AM

ping

tra accepted this revision.Feb 26 2019, 1:43 PM

LGTM.

This revision is now accepted and ready to land.Feb 26 2019, 1:43 PM
Closed by commit rC354929: [CUDA][HIP] Check calling convention based on function target (authored by yaxunl). · Explain WhyFeb 26 2019, 2:27 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2019, 2:27 PM
rjmccall added inline comments.Feb 26 2019, 10:12 PM
lib/Sema/SemaDeclAttr.cpp
4619–4641

Please sink these declarations into the CUDA-specific block. Also, please add some comments to explain why different logic is needed for CUDA.

yaxunl marked an inline comment as done.Feb 27 2019, 6:57 AM
yaxunl added inline comments.
lib/Sema/SemaDeclAttr.cpp
4619–4641

will do. thanks.

yaxunl marked an inline comment as done.Feb 27 2019, 7:08 AM
yaxunl added inline comments.
lib/Sema/SemaDeclAttr.cpp
4619–4641

variables A and TI are also used by else part of the if statement, so only variable Aux can be moved in the if part.

rjmccall added inline comments.Feb 27 2019, 10:07 AM
lib/Sema/SemaDeclAttr.cpp
4619–4641

Yes, sorry, only your new declaration needs to be sunk.

Revision Contents

PathSize
lib/
Sema/
25 lines
test/
SemaCUDA/
11 lines

Diff 185375

lib/Sema/SemaDeclAttr.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 4,608 Lines • ▼ Show 20 Lines case ParsedAttr::AT_PreserveMost:
CC = CC_PreserveMost; CC = CC_PreserveMost;
break; break;
case ParsedAttr::AT_PreserveAll: case ParsedAttr::AT_PreserveAll:
CC = CC_PreserveAll; CC = CC_PreserveAll;
break; break;
default: llvm_unreachable("unexpected attribute kind"); default: llvm_unreachable("unexpected attribute kind");
} }
TargetInfo::CallingConvCheckResult A = TargetInfo::CCCR_OK;
const TargetInfo &TI = Context.getTargetInfo(); const TargetInfo &TI = Context.getTargetInfo();
TargetInfo::CallingConvCheckResult A = TI.checkCallingConvention(CC); auto *Aux = Context.getAuxTargetInfo();
if (LangOpts.CUDA) {
auto CUDATarget = IdentifyCUDATarget(FD);
if (CUDATarget == CFT_HostDevice) {
A = TI.checkCallingConvention(CC);
if (A == TargetInfo::CCCR_OK && Aux)
A = Aux->checkCallingConvention(CC);
} else if (LangOpts.CUDAIsDevice) {
if (CUDATarget == CFT_Device || CUDATarget == CFT_Global) {
A = TI.checkCallingConvention(CC);
} else if (CUDATarget == CFT_Host && Aux) {
A = Aux->checkCallingConvention(CC);
}
} else {
if ((CUDATarget == CFT_Device || CUDATarget == CFT_Global) && Aux) {
A = Aux->checkCallingConvention(CC);
} else if (CUDATarget == CFT_Host) {
A = TI.checkCallingConvention(CC);
}
}
traUnsubmitted
Not Done
ReplyInline Actions

This is rather convoluted.
Perhaps it would be easier to understand if the code is restructured. along these lines:

// Returns a tuple indicating whether we need to check TargetInfo on host/device side of the compilation
bool CheckHost = false, CheckDevice=false;

switch (CudaTarget) { 
    case CFT_HostDevice: 
      CheckHost = true; CheckDevice=true; break;
    case CFT_Host: 
      CheckHost = true; break;
    case CFG_Device: 
    case CFG_Global: 
      CheckDevice=true; break;
  }
} ();
TargetInfo *HostTI = CudaIsDevice ? Aux : &TI;
TargetInfo * DeviceTI = CudaIsDevice ? &TI : Aux;
if (CheckHost && HostTI)
  HostTI->checkCallingConvention(CC);
if (CheckDevice && DeviceTI)
    DeviceTI->checkCallingConvention(CC);
tra: This is rather convoluted. Perhaps it would be easier to understand if the code is restructured.
} else {
A = TI.checkCallingConvention(CC);
}
rjmccallUnsubmitted
Not Done
ReplyInline Actions

Please sink these declarations into the CUDA-specific block. Also, please add some comments to explain why different logic is needed for CUDA.

rjmccall: Please sink these declarations into the CUDA-specific block. Also, please add some comments to…
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

will do. thanks.

yaxunl: will do. thanks.
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

variables A and TI are also used by else part of the if statement, so only variable Aux can be moved in the if part.

yaxunl: variables A and TI are also used by else part of the if statement, so only variable Aux can be…
rjmccallUnsubmitted
Not Done
ReplyInline Actions

Yes, sorry, only your new declaration needs to be sunk.

rjmccall: Yes, sorry, only your new declaration needs to be sunk.
if (A != TargetInfo::CCCR_OK) { if (A != TargetInfo::CCCR_OK) {
if (A == TargetInfo::CCCR_Warning) if (A == TargetInfo::CCCR_Warning)
Diag(Attrs.getLoc(), diag::warn_cconv_ignored) << Attrs; Diag(Attrs.getLoc(), diag::warn_cconv_ignored) << Attrs;
// This convention is not valid for the target. Use the default function or // This convention is not valid for the target. Use the default function or
// method calling convention. // method calling convention.
bool IsCXXMethod = false, IsVariadic = false; bool IsCXXMethod = false, IsVariadic = false;
if (FD) { if (FD) {
▲ Show 20 Lines • Show All 3,809 Lines • Show Last 20 Lines

test/SemaCUDA/amdgpu-windows-vectorcall.cu

  • This file was added.
// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -aux-triple x86_64-pc-windows-msvc -fms-compatibility -fcuda-is-device -fsyntax-only -verify %s
// expected-no-diagnostics
traUnsubmitted
Not Done
ReplyInline Actions

It may be good to add a check where we *would* expect to see the diagnostics.

tra: It may be good to add a check where we *would* expect to see the diagnostics.
traUnsubmitted
Not Done
ReplyInline Actions

It may be good to add a check where we *would* expect to see the diagnostics.

Scratch that. Wrong calling convention attribute will be ignored.

tra: > It may be good to add a check where we *would* expect to see the diagnostics. Scratch that.
template<class _Ty>
struct A
{
};
template<class _Ret, class _Arg0, class _Types> struct A<_Ret (__cdecl _Arg0::*)(_Types) > { };
template<class _Ret, class _Arg0, class _Types> struct A<_Ret (__vectorcall _Arg0::*)(_Types) > {};
traUnsubmitted
Not Done
ReplyInline Actions

I don't think we need templates here. We're only making sure that the function attributes are handled correctly.
Can we get by with a regular function declaration or definition?

tra: I don't think we need templates here. We're only making sure that the function attributes are…
yaxunlAuthorUnsubmitted
Done
ReplyInline Actions

Here we want to make sure _Ret (__cdecl _Arg0::*)(_Types) and _Ret (__vectorcall _Arg0::*)(_Types) are different types because calling convention is part of type, otherwise it results in compilation error due to duplicate template instantation. I can come up with a test without template.

yaxunl: Here we want to make sure `_Ret (__cdecl _Arg0::*)(_Types)` and `_Ret (__vectorcall _Arg0::*)…