This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Support `__managed__` attribute
ClosedPublic

Authored by yaxunl on Jan 15 2021, 11:45 AM.

Details

Summary

This patch implements codegen for __managed__ variable attribute for HIP.

Diagnostics will be added later.

Diff Detail

Event Timeline

yaxunl created this revision.Jan 15 2021, 11:45 AM
yaxunl requested review of this revision.Jan 15 2021, 11:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 11:45 AM
tra added a comment.Jan 19 2021, 1:20 PM

Presumably, __managed__ variables would have to be memory-mapped into the host address space.

clang/lib/CodeGen/CGCUDANV.cpp
391

VarUse ?

395

WorkItem

552–553

This will always be false, because all cases where Info.Flags.isManaged() is true are handled in the other branch of this if.

clang/lib/Sema/SemaDeclAttr.cpp
7823

The code changes in the patch appear to treat __managed__ variable as a superset of a __device__ var. If that's indeed the case, adding an implicit __device__ attribute here would help to simplify the code. This way the existing code can handle generic __device__ var functionality without additional changes, and would use __managed__ checks for the cases specific for managed vars only.

llvm/lib/IR/ReplaceConstant.cpp
21

The function could use a descriptive comment.

Despite the comment above, it does seem to do any replacing itself. Presumably it creates a an instruction that would perform an equivalent calculation at runtime.

aaron.ballman added inline comments.Jan 20 2021, 7:05 AM
clang/include/clang/Basic/AttrDocs.td
5426
5427–5429
clang/lib/Sema/SemaDeclAttr.cpp
7823

I think you're missing changes to the CUDA global attr to check for mutual exclusions with __managed__ as well. Also, I think this won't do the right thing for redeclarations, like:

__device__ extern int i;
__managed__ extern int i;
7823

This way the existing code can handle generic device var functionality without additional changes, and would use managed checks for the cases specific for managed vars only.

Another alternative to consider is to not create a new semantic attribute named HIPManagedAttr but to instead add a new spelling to CUDADevice in Attr.td, giving the class an Accessor to distinguish which spelling the user wrote in code, and use that accessor for the specific cases for managed vars.

yaxunl marked 9 inline comments as done.Jan 21 2021, 9:20 AM
yaxunl added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
7823

I will add an implicit device attr for managed attr. I think this will have a more readable AST.

7823

Actually __managed__ does not need exclude with __global__ since __global__ cannot be used with variable. However, __managed__ need to exclude with __constant__ and __shared__. I will add handling for that.

__managed__ should behave the same as __device__ regarding compatibility with extern. It is allowed to be with extern. Currently for extern __device__ var, on device side, it is treated as a normal device variable with definition; on host side, the shadow variable is emitted with internal linkage. This is fine for the default -fno-gpu-rdc mode. However, this does not work for -fgpu-rdc mode. For -fgpu-rdc mode, both device variable and shadow variable should be declarations. Since this issue is orthogonal to __managed__, I will create a separate patch to fix it.

yaxunl updated this revision to Diff 318243.Jan 21 2021, 9:35 AM
yaxunl marked an inline comment as done.

revised by Artem's and Aaron's comments.

tra accepted this revision.Jan 21 2021, 9:55 AM

LGTM.

This revision is now accepted and ready to land.Jan 21 2021, 9:55 AM
aaron.ballman accepted this revision.Jan 22 2021, 6:04 AM

The attribute parts LGTM aside from some small nits.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8250–8251

Since we're modifying this line anyway, can you wrap it for the 80-col limit?

clang/test/SemaCUDA/managed-var.cu
46

I think you're missing tests that check that the new managed attribute accepts no arguments and that it doesn't apply to things other than variables (like a function declaration).

yaxunl marked 2 inline comments as done.Jan 22 2021, 8:23 AM
yaxunl added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8250–8251

will fix when committing

clang/test/SemaCUDA/managed-var.cu
46

will add tests when committing

This revision was automatically updated to reflect the committed changes.
yaxunl marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2021, 8:47 AM