Page MenuHomePhabricator

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

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

Diff Detail

Repository
rC Clang

Event Timeline

yaxunl created this revision.Feb 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
4622–4639

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
4

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.

tra added inline comments.Feb 21 2019, 2:25 PM
test/SemaCUDA/amdgpu-windows-vectorcall.cu
4

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.

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
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
4620

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
4620

will do. thanks.

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

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
4620

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