[HIP] Make sure, unused hip-pinned-shadow global var is kept within device code

[hsmhsm]

Summary

hip-pinned-shadow global var should remain in the final code object irrespective
of whether it is used or not within the code. Add it to used list, so that it
will not get eliminated when it is unused.

[yaxunl]

LGTM. Thanks.

[hliao]

the assertion in addUsedGlobal should not be removed. that will remove the guard for potential bugs

clang/lib/CodeGen/CodeGenModule.cpp

1918–1921

This check should be removed completely instead it should be revised for the exceptions.

[yaxunl]

clang/lib/CodeGen/CodeGenModule.cpp

1918–1921

How about add back this assertion but make an exception for global variables with hip_pinned_shadow attribute.

[hsmhsm]

clang/lib/CodeGen/CodeGenModule.cpp

1918–1921

Yeah, that is what I was thinking of initially - to remove this assertion only to hip_pinned_shadow vars, may, be creating a clone of this function something like - CodeGenModule::addUsedGlobalForHipPinnedShadows(), without assertion statement.

@hliao

What do you say?

[hliao]

clang/lib/CodeGen/CodeGenModule.cpp

1918–1921

how about change that function prototype to

void addUsedGlobal(llvm::GlobalValue *GV, bool SkipCheck = false)

and assert check to

assert ((SkipCheck || !GV->isDeclaration()) && "");

[hliao]

BTW, why that variable cannot have an initializer? Suppose that initializer is a trivial one, initializing to 0, would that cause any issue in the compilation?

[hsmhsm]

clang/lib/CodeGen/CodeGenModule.cpp

1918–1921

This also looks fine to me, and I can make these changes and submit the changes.

[hsmhsm]

In D75402#1901361, @hliao wrote:

BTW, why that variable cannot have an initializer? Suppose that initializer is a trivial one, initializing to 0, would that cause any issue in the compilation?

Initialization makes the global extern var declaration to become a global definition. And. moreover, it is not a new change from my side, I just happen to add a comment.

[yaxunl]

In D75402#1901370, @hsmhsm wrote:

In D75402#1901361, @hliao wrote:

BTW, why that variable cannot have an initializer? Suppose that initializer is a trivial one, initializing to 0, would that cause any issue in the compilation?

Initialization makes the global extern var declaration to become a global definition. And. moreover, it is not a new change from my side, I just happen to add a comment.

This kind of variable is supposed to be an external variable which is initialized by runtime at run time.

[hsmhsm]

Take care review comments by hliao.

[hsmhsm]

In D75402#1901471, @hsmhsm wrote:

Take care review comments by hliao.

ping

[yaxunl]

LGTM. Thanks!

clang/lib/CodeGen/CodeGenModule.h

public:
 
   /// If the declaration has internal linkage but is inside an
   /// extern "C" linkage specification, prepare to emit an alias for it
   /// to the expected name.
   template<typename SomeDecl>
   void MaybeHandleStaticInExternC(const SomeDecl *D, llvm::GlobalValue *GV);
 
   /// Add a global to a list to be added to the llvm.used metadata.
-  void addUsedGlobal(llvm::GlobalValue *GV);
+  void addUsedGlobal(llvm::GlobalValue *GV, bool SkipCheck = false);
 
   /// Add a global to a list to be added to the llvm.compiler.used metadata.
   void addCompilerUsedGlobal(llvm::GlobalValue *GV);
 
   /// Add a destructor and object to add to the C++ global destructor function.
   void AddCXXDtorEntry(llvm::FunctionCallee DtorFn, llvm::Constant *Object) {
     CXXGlobalDtors.emplace_back(DtorFn.getFunctionType(), DtorFn.getCallee(),
                                 Object);

clang/lib/CodeGen/CodeGenModule.cpp

if (const auto *CB = FD->getAttr<CallbackAttr>()) {
     int CalleeIdx = *CB->encoding_begin();
     ArrayRef<int> PayloadIndices(CB->encoding_begin() + 1, CB->encoding_end());
     F->addMetadata(llvm::LLVMContext::MD_callback,
                    *llvm::MDNode::get(Ctx, {MDB.createCallbackEncoding(
                                                CalleeIdx, PayloadIndices,
                                                /* VarArgsArePassed */ false)}));
   }
 }
 
-void CodeGenModule::addUsedGlobal(llvm::GlobalValue *GV) {
-  assert(!GV->isDeclaration() &&
-         "Only globals with definition can force usage.");
+void CodeGenModule::addUsedGlobal(llvm::GlobalValue *GV, bool SkipCheck) {
+  assert(SkipCheck || (!GV->isDeclaration() &&
+                       "Only globals with definition can force usage."));
   LLVMUsed.emplace_back(GV);
 }
 
 void CodeGenModule::addCompilerUsedGlobal(llvm::GlobalValue *GV) {
   assert(!GV->isDeclaration() &&
          "Only globals with definition can force usage.");
   LLVMCompilerUsed.emplace_back(GV);
 }
if (LangOpts.CUDAIsDevice) {
         // they are not registered with the CUDA runtime, so they
         // can't really be used to access their device-side
         // counterparts. It's not clear yet whether it's nvcc's bug or
         // a feature, but we've got to do the same for compatibility.
         Linkage = llvm::GlobalValue::InternalLinkage;
     }
   }
 
-  if (!IsHIPPinnedShadowVar)
+  // HIPPinnedShadowVar should remain in the final code object irrespective of
+  // whether it is used or not within the code. Add it to used list, so that
+  // it will not get eliminated when it is unused. Also, it is an extern var
+  // within device code, and it should *not* get initialized within device code.
+  if (IsHIPPinnedShadowVar)
+    addUsedGlobal(GV, /*SkipCheck=*/true);
+  else
     GV->setInitializer(Init);
-  if (emitter) emitter->finalize(GV);
+
+  if (emitter)
+    emitter->finalize(GV);
 
   // If it is safe to mark the global 'constant', do so now.
   GV->setConstant(!NeedsGlobalCtor && !NeedsGlobalDtor &&
                   isTypeConstant(D->getType(), true));
 
   // If it is in a read-only section, mark it 'constant'.
   if (const SectionAttr *SA = D->getAttr<SectionAttr>()) {
     const ASTContext::SectionInfo &SI = Context.SectionInfos[SA->getName()];

clang/test/CodeGenCUDA/hip-pinned-shadow.cu

 // REQUIRES: amdgpu-registered-target
 
 // RUN: %clang_cc1 -triple amdgcn -fcuda-is-device -std=c++11 -fvisibility hidden -fapply-global-visibility-to-externs \
 // RUN:     -emit-llvm -o - -x hip %s | FileCheck -check-prefixes=HIPDEV %s
 // RUN: %clang_cc1 -triple x86_64 -std=c++11 \
 // RUN:     -emit-llvm -o - -x hip %s | FileCheck -check-prefixes=HIPHOST %s
+// RUN: %clang_cc1 -triple amdgcn -fcuda-is-device -std=c++11 -fvisibility hidden -fapply-global-visibility-to-externs \
+// RUN:     -O3 -emit-llvm -o - -x hip %s | FileCheck -check-prefixes=HIPDEVUNSED %s
 
 struct textureReference {
   int a;
 };
 
 template <class T, int texType, int hipTextureReadMode>
 struct texture : public textureReference {
 texture() { a = 1; }
 };
 
 __attribute__((hip_pinned_shadow)) texture<float, 2, 1> tex;
 // CUDADEV-NOT: @tex
 // CUDAHOST-NOT: call i32 @__hipRegisterVar{{.*}}@tex
 // HIPDEV: @tex = external addrspace(1) global %struct.texture
 // HIPDEV-NOT: declare{{.*}}void @_ZN7textureIfLi2ELi1EEC1Ev
 // HIPHOST:  define{{.*}}@_ZN7textureIfLi2ELi1EEC1Ev
 // HIPHOST:  call i32 @__hipRegisterVar{{.*}}@tex{{.*}}i32 0, i32 4, i32 0, i32 0)
+// HIPDEVUNSED: @tex = external addrspace(1) global %struct.texture
+// HIPDEVUNSED-NOT: declare{{.*}}void @_ZN7textureIfLi2ELi1EEC1Ev