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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
