[PATCH] D62738: [HIP] Support device_shadow variable

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: include/clang/Basic/Attr.td:954
 
+def CUDADeviceShadow : InheritableAttr {
+  let Spellings = [GNU<"device_shadow">, Declspec<"__device_shadow__">];
----------------
`HIPDeviceShadow` ?


================
Comment at: include/clang/Basic/Attr.td:955
+def CUDADeviceShadow : InheritableAttr {
+  let Spellings = [GNU<"device_shadow">, Declspec<"__device_shadow__">];
+  let Subjects = SubjectList<[Var]>;
----------------
In light of the details you've provided below, perhaps this needs a better name. I've suggested `__device_shadow__` without being aware of what exactly it's supposed to do in HIP.  
Perhaps something like `__hip_device_shadow__` or `__hip_pinned_shadow` ? Naming is hard. :-)


================
Comment at: include/clang/Basic/Attr.td:957
+  let Subjects = SubjectList<[Var]>;
+  let LangOpts = [CUDA];
+  let Documentation = [DeviceShadowDocs];
----------------
Shis should probably be `[HIP]` now, too.


================
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.
+
----------------
yaxunl wrote:
> tra wrote:
> > 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.
> > 
> If only address and size of such variables should be used on device side, such variables will not be very useful.
> 
> To implement texture reference, we need to be able to load the device side shadow variable. In general, it is desirable to load and store device side shadow variables, since users have no other way to synch with the corresponding host variable in device code.
> 
> This is different from host side shadow variable. On host side, users can use hipMemcpyToSymbol and hipMemcpyFromSymbol to force synchronization between the host side shadow variable and the corresponding device variable.
> 
> Therefore the implementation of the device side shadow variable requires special handling in HIP runtime. Basically HIP runtime needs to pin the host variable and use it to resolve the device side shadow variable (as an undefined elf symbol). This way, the host variable and device side shadow variable are sharing the same memory. This is also why it is HIP specific since CUDA runtime may not have such handling.
> 
> 
Thank you for providing the details. This use case is sufficiently different from the more general purpose reverse-shadow-var I had in mind.



================
Comment at: lib/CodeGen/CodeGenModule.cpp:3775
+  // left undefined.
+  bool IsHIPDeviceShadowVar = getLangOpts().HIP && getLangOpts().CUDAIsDevice &&
+                              D->hasAttr<CUDADeviceShadowAttr>();
----------------
yaxunl wrote:
> 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.
There's currently no specific use case for it in CUDA and HIP's use case also does not quite fit this, so `__shadow_variable__` is not going to do either of us any good. So, we can drop the `__shadow_variable__`.


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

https://reviews.llvm.org/D62738