ABataev added inline comments.
================
Comment at: lib/CodeGen/CGExpr.cpp:2297-2319
+static Address emitDeclTargetToVarDeclLValue(CodeGenFunction &CGF,
+ const VarDecl *VD, QualType T) {
+ llvm::Optional<OMPDeclareTargetDeclAttr::MapTypeTy> Res =
+ OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(VD);
+ if (!Res || *Res == OMPDeclareTargetDeclAttr::MT_Link)
+ return Address::invalid();
+ assert(*Res == OMPDeclareTargetDeclAttr::MT_To && "Expected to clause");
----------------
I think it would be better to merge these 2 functions into 1
`emitDeclTargetVarDeclLValue`. It should return the correct address for link
vars and to vars with unified memory.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7458-7467
if (*Res == OMPDeclareTargetDeclAttr::MT_Link) {
IsLink = true;
BP = CGF.CGM.getOpenMPRuntime().getAddrOfDeclareTargetLink(VD);
}
+ if (*Res == OMPDeclareTargetDeclAttr::MT_To &&
+ CGF.CGM.getOpenMPRuntime().hasRequiresUnifiedSharedMemory()) {
+ // TODO: Make this into a flag for TO with unified memory.
----------------
You can merge those pieces of code into one, no need to duplicate. Plus, I
don't think we need a new flag for to with unified memory if we can express
everything using the existing flags.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:1123-1127
+ virtual Address getAddrOfDeclareTargetToUnderUnifiedMem(const VarDecl *VD);
+
/// Returns the address of the variable marked as declare target with link
/// clause.
virtual Address getAddrOfDeclareTargetLink(const VarDecl *VD);
----------------
Same here, better to merge these 2 functions into one
`getAddrOfDeclareTargetVar`
================
Comment at: test/OpenMP/nvptx_target_requires_unified_shared_memory.cpp:2
// Test declare target link under unified memory requirement.
// RUN: %clang_cc1 -verify -fopenmp -fopenmp-cuda-mode -x c++ -triple
powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s
-o - | FileCheck %s --check-prefix CHECK
// expected-no-diagnostics
----------------
`CHECK` is the default prefix, no need to specify it.
================
Comment at: test/OpenMP/nvptx_target_requires_unified_shared_memory.cpp:2
// Test declare target link under unified memory requirement.
// RUN: %clang_cc1 -verify -fopenmp -fopenmp-cuda-mode -x c++ -triple
powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s
-o - | FileCheck %s --check-prefix CHECK
// expected-no-diagnostics
----------------
ABataev wrote:
> `CHECK` is the default prefix, no need to specify it.
We also need the checks for the device codegen since you're changing something
not only in the host codegen, but in the device codegen too. Just extend this
test to check for the codegen for the device.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63108/new/
https://reviews.llvm.org/D63108
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
