[PATCH] D62738: [HIP] Support device_shadow variable

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Overall looks good. Thank you for making the change.

While reviewing the patch it occured to me that it presents an opportunity to generalize the shadow variables to work in both directions. See below.



================
Comment at: include/clang/Basic/AttrDocs.td:4164-4171
+The GNU style attribute __attribute__((device_shadow)) or MSVC style attribute
+__declspec(device_shadow) can be added to the definition of a global variable
+to indicate it is a HIP device shadow variable. A device shadow variable can
+be accessed on both device side and host side. It has external linkage and is
+not initialized on device side. It has internal linkage and is initialized by
+the initializer on host side.
+
----------------
just `device shadow variable` would do. It's no longer, generally speaking, HIP-specific. :-)

Only address and size of such variables should be used on device side.

I'd rephrase the use constraint. Currently it's `!(CUDA || !CUDA)` which is always false.
`Currently enabled for HIP only.` would be closer to reality.



================
Comment at: lib/CodeGen/CodeGenModule.cpp:3775
+  // left undefined.
+  bool IsHIPDeviceShadowVar = getLangOpts().HIP && getLangOpts().CUDAIsDevice &&
+                              D->hasAttr<CUDADeviceShadowAttr>();
----------------
`IsDeviceShadowVar`. We may want to rename `IsCUDAShadowVar` to `IsHostShadowVar` to be consistent.

This got me thinking. Conceptually we have two different things going on here.
* where do we place the real variable
* whether we need to create a shadow on the other end.

Currently `__device__`, `__constant__` and `__shared__` act as both.
This patch implements the same `make a shadow on the other side`, only in the opposite direction.

Perhaps the right thing to do is to push the patch even further and make it into a `__shadow_variable__` which will be responsible for creating the other side shadow and would work in both directions.

We can then assign implicit `__shadow_variable__` attribute to the device-side vars to preserve current behavior and it will work for your purposes two. We will also gain ability to create device-side variables w/o host-side shadows, if we need to.

I guess in the end it would be this patch + a bit of refactoring/collapsing of `IsCUDAShadowVar` logic.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62738/new/

https://reviews.llvm.org/D62738