variables emitted on both host and device side with different addresses
when ODR-used by host function should not cause device side counter-part
to be force emitted.
This fixes the regression caused by https://reviews.llvm.org/D102237
Paths
| Differential D102801
[CUDA][HIP] Fix device variables used by host ClosedPublic Authored by yaxunl on May 19 2021, 12:55 PM.
Details Summary variables emitted on both host and device side with different addresses This fixes the regression caused by https://reviews.llvm.org/D102237
Diff Detail
Unit TestsFailed Event TimelineComment Actions Tentative LGTM as we need it to fix the regression soon. Summoning @rsmith for the 'big picture' opinion.
Comment Actions This patch does not appear to fix the second regression introduced by the D102237. Trying to compile the following code triggers an assertion in CGExpr.cpp: class a { public: a(char *); }; void b() { [](char *c) { static a d(c); d; }; } With assertions disabled it eventually leads to a different error: yaxunl retitled this revision from [CUDA][HIP] Fix implicit constant variable to [CUDA][HIP] Fix device variables used by host. Comment ActionsFix the other regression Comment Actions
The root cause is similar to the last regression. Basically when a variable is emitted on both sides but as different entities, we should not treat it as a device variable on host side. I have updated the patch to fix both regressions.
Comment Actions
In the updated patch I have a simpler solution which is easier to explain to the users. Basically we classify variables by how they are emitted: device side only, host side only, both sides as different entities (e.g. default constexpr var), and both sides as unified entity (e.g. managed var). For variables emitted on both sides as separate entities, we have limited knowledge and we limit what we can do for them. I think users should understand the compiler's limitation in such cases. And they can easily workaround that by making the variable explicitly device variable. Comment Actions
This is really nice. Let me test it internally and see if anything breaks.
yaxunl added inline comments.
Comment Actions LGTM. I've verified that Tensorflow still builds with this patch and that the patch does fix the regressions we've seen. This revision is now accepted and ready to land.May 20 2021, 1:31 PM This revision was landed with ongoing or failed builds.May 20 2021, 2:04 PM Closed by commit rG4cb42564ec4b: [CUDA][HIP] Fix device variables used by host (authored by yaxunl). · Explain Why This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 346831 clang/include/clang/Sema/Sema.h
clang/lib/CodeGen/CGDeclCXX.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/Sema/SemaCUDA.cpp
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/AST/ast-dump-constant-var.cu
clang/test/CodeGenCUDA/host-used-device-var.cu
clang/test/SemaCUDA/static-device-var.cu
|
Wasn't there another kind, where the variable is emitted on the host with device-side shadow? I vaguely recall it had something to do with textures.