This is an archive of the discontinued LLVM Phabricator instance.

[Clang][OpenMP][AMDGPU] Fix capture of variably modified type alias in teams distribute
Needs ReviewPublic

Authored by doru1004 on Nov 23 2022, 2:40 PM.

Details

Summary

This patch fixes an issue with failing to properly capture a variably modified type alias of an already mapped flat array. The test showcases a minimal example of the failing scenario.

Diff Detail

Event Timeline

doru1004 created this revision.Nov 23 2022, 2:40 PM
doru1004 requested review of this revision.Nov 23 2022, 2:40 PM
Herald added a project: Restricted Project. · View Herald Transcript

LGTM but @ABataev should review/approve this

ABataev added inline comments.Nov 29 2022, 9:52 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

Is it for pointers only? Or for other types too?

doru1004 added inline comments.Nov 29 2022, 10:29 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

I am not sure about that distinction; it allows for the same types as before except that when the directive is not a combined parallel for directive it returns immediately because the variable will then have to be captured and shared.

doru1004 added inline comments.Nov 29 2022, 10:34 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

Correction: it allows for VMTs to pass through in cases in which you don't use a combined parallel for directive.

ABataev added inline comments.Nov 29 2022, 10:49 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

IsVMTTy?

243–244

How can it be captured by value? Why does it happen? My first question relates to the assertion you changed. I assume, it applies only to the pointers. Otherwise, please add more context how we can capture mariably modifiable type by value.

doru1004 added inline comments.Nov 29 2022, 12:26 PM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

Yes it should be called IsVMTTy

doru1004 updated this revision to Diff 478693.Nov 29 2022, 12:54 PM
ABataev added inline comments.Nov 29 2022, 1:19 PM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

Still naming, IsVMTTy.

doru1004 updated this revision to Diff 478707.Nov 29 2022, 1:25 PM
doru1004 added inline comments.Nov 30 2022, 7:13 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

It only applies to pointers, it's the pointer that needs to be used correctly.

doru1004 marked an inline comment as done.Dec 5 2022, 7:04 AM
ABataev added inline comments.Dec 13 2022, 6:12 AM
clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
243–244

Can you change the assert check then to something like this:

assert((VD->getType()->isPointerType() || !VD->getType()->isVariablyModifiedType()) &&
246–249

Also, can you make it specific to pinter types?