[PATCH] D62738: [HIP] Support device_shadow variable

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:3775
+  // left undefined.
+  bool IsHIPDeviceShadowVar = getLangOpts().HIP && getLangOpts().CUDAIsDevice &&
+                              D->hasAttr<CUDADeviceShadowAttr>();
----------------
tra wrote:
> `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.
Do we really want to introduce a generic `__shadow_variable__` for device variables? It has little use but complicates AST of device variables unnecessarily. First it bring no new functionality since device variables are already shadowed by default. Second since unused shadow variable is eliminated automatically due to their internal linkage, disable shadowing will not save memory in host binary.


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

https://reviews.llvm.org/D62738